RE: [PATCH v2] sh_eth: remove unchecked interrupts
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
[PATCH v3] sh_eth: remove unchecked interrupts for RZ/A1
When streaming a lot of data and the RZ/A1 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 <chris.bra...@renesas.com>
Acked-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
---
v3:
* add RZ/A1 to subject line
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(-)
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 |
--
2.10.1
RE: [PATCH v2] sh_eth: remove unchecked interrupts
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
[PATCH v2] sh_eth: remove unchecked interrupts
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 <chris.bra...@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(-)
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 |
--
2.10.1
RE: [PATCH] sh_eth: add missing checks for status bits
On 12/1/2016, Sergei Shtylyov wrote: > Hello! > > Please always CC me on the sh_eth/ravb driver patches as directed by > scripts/get_maintainer.pl. OK. I'm sorry. > On 11/30/2016 11:01 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. > > Perhaps we should just clear the correspoding bits in EESIPR instead? > They are not set for any other SoC... That's a good point. If we don't plan on doing anything with those bits, they should not be causing interrupts. I will try change and then re-test. Chris
[PATCH] sh_eth: add missing checks for status bits
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 <chris.bra...@renesas.com>
---
drivers/net/ethernet/renesas/sh_eth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/renesas/sh_eth.c
b/drivers/net/ethernet/renesas/sh_eth.c
index 05b0dc5..079f10e 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -523,7 +523,7 @@ static struct sh_eth_cpu_data r7s72100_data = {
.tx_check = EESR_TC1 | EESR_FTC,
.eesr_err_check = EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |
EESR_RFE | EESR_RDE | EESR_RFRMER | EESR_TFE |
- EESR_TDE | EESR_ECI,
+ EESR_TDE | EESR_ECI | EESR_TUC | EESR_ROC,
.fdr_value = 0x070f,
.no_psr = 1,
--
2.10.1
RE: [PATCH v2] net: ethernet: renesas: sh_eth: add POST registers for rz
On 9/9/2016, Sergei Shtylyov wrote:
> > sh_eth_private *mdp) {
> > if (sh_eth_is_rz_fast_ether(mdp)) {
> > sh_eth_tsu_write(mdp, 0, TSU_TEN); /* Disable all CAM entry */
> > + sh_eth_tsu_write(mdp, TSU_FWSLC_POSTENU | TSU_FWSLC_POSTENL,
> > +TSU_FWSLC);/* Enable POST registers */
> > return;
> > }
>
> Wait, don't you also need to write 0s to the POST registers like done
> at the end of this function?
Nope.
The sh_eth_chip_reset() function will write to register ARSTR which will do a
HW reset on the block and clear all the registers, including all the POST
registers.
static struct sh_eth_cpu_data r7s72100_data = {
.chip_reset = sh_eth_chip_reset,
So, before sh_eth_tsu_init() is ever called, the hardware will always be reset.
/* initialize first or needed device */
if (!devno || pd->needs_init) {
if (mdp->cd->chip_reset)
mdp->cd->chip_reset(ndev);
if (mdp->cd->tsu) {
/* TSU init (Init only)*/
sh_eth_tsu_init(mdp);
}
}
Therefore there is no reason to set the POST registers back to 0 because they
are already at 0 from the reset.
Chris
RE: [PATCH] net: ethernet: renesas: sh_eth: do not access POST registers if not exist
As a follow up on this thread: On 8/30/2016, Geert Uytterhoeven wrote: > > I just looked at the RZ/A1 register space and there seems to be dummy > > registers in the POST1-4 area. I can write to them and read back what I > > wrote...which is all that the sh_eth driver cares about. I bet when the > > designers bring in the EtherC IP block, the entire register address is > > always populated with a simple latch registers. And then, if a feature is > > not included (like relay/POST), then nothing is hooked up on the back side > > of it. > > What a waste of transistors... > There are plenty of hidden registers in many IP blocks. Well Geert, turns out those transistors weren't wasted. They are there...and in fact are need to actually make the system work correctly. After some investigation, looks like when someone was cutting out un-needed pages to make the RZ/A1 hardware manual they misunderstood what those registers were for. In reality, they just needed to copy the pages from the SH7734 Hardware manual for the POST1-4. Also, if you read about the TSU closely you'll see that you also need register FWSLC to actually enable the POST1-4 registers (more or less). I've already submitted a v2 patch which should be pretty straight forward now. Chris
[PATCH v2] net: ethernet: renesas: sh_eth: add POST registers for rz
Due to a mistake in the hardware manual, the FWSLC and POST1-4 registers
were not documented and left out of the driver for RZ/A making the CAM
feature non-operational.
Additionally, when the offset values for POST1-4 are left blank, the driver
attempts to set them using an offset of 0x which can cause a memory
corruption or panic.
This patch fixes the panic and properly enables CAM.
Reported-by: Daniel Palmer <dan...@0x0f.com>
Signed-off-by: Chris Brandt <chris.bra...@renesas.com>
---
v2:
* POST registers really do exist, so just add them
---
drivers/net/ethernet/renesas/sh_eth.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/ethernet/renesas/sh_eth.c
b/drivers/net/ethernet/renesas/sh_eth.c
index 1f8240a..440ae27 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -201,9 +201,14 @@ static const u16
sh_eth_offset_fast_rz[SH_ETH_MAX_REGISTER_OFFSET] = {
[ARSTR] = 0x,
[TSU_CTRST] = 0x0004,
+ [TSU_FWSLC] = 0x0038,
[TSU_VTAG0] = 0x0058,
[TSU_ADSBSY]= 0x0060,
[TSU_TEN] = 0x0064,
+ [TSU_POST1] = 0x0070,
+ [TSU_POST2] = 0x0074,
+ [TSU_POST3] = 0x0078,
+ [TSU_POST4] = 0x007c,
[TSU_ADRH0] = 0x0100,
[TXNLCR0] = 0x0080,
@@ -2781,6 +2786,8 @@ static void sh_eth_tsu_init(struct sh_eth_private *mdp)
{
if (sh_eth_is_rz_fast_ether(mdp)) {
sh_eth_tsu_write(mdp, 0, TSU_TEN); /* Disable all CAM entry */
+ sh_eth_tsu_write(mdp, TSU_FWSLC_POSTENU | TSU_FWSLC_POSTENL,
+TSU_FWSLC);/* Enable POST registers */
return;
}
--
2.9.2
RE: [PATCH] net: ethernet: renesas: sh_eth: do not access POST registers if not exist
Hello Sergei, On Aug 29, 2016, Sergei Shtylyov wrote: >SH7757 is not the only platform with TSU, there's e.g. R8A7740 ARM > SoC which only has 1 GETHER channel... I don't have the R8A7740 manual (R-Mobile A1) so I can't see. But even if it does not have the POST registers, it might not hurt anything. I just looked at the RZ/A1 register space and there seems to be dummy registers in the POST1-4 area. I can write to them and read back what I wrote...which is all that the sh_eth driver cares about. I bet when the designers bring in the EtherC IP block, the entire register address is always populated with a simple latch registers. And then, if a feature is not included (like relay/POST), then nothing is hooked up on the back side of it. So, the 'easiest' solution is to just put the registers into the sh_eth_offset_fast_rz array and not try to make things more complicated. [TSU_TEN] = 0x0064, + [TSU_POST1] = 0x0070, + [TSU_POST2] = 0x0074, + [TSU_POST3] = 0x0078, + [TSU_POST4] = 0x007c, Chris
RE: [PATCH] net: ethernet: renesas: sh_eth: do not access POST registers if not exist
On 08/28/2016, Sergei Shtylyov wrote: >> The RZ/A1 has a TSU, but since it only has one Ethernet port, it does >> not have POST registers. > >I'm not sure the reason is having one port... do you have the old SH > manuals somewhere? :-) Yes, I used to support the SH7757. It had dual ETHER and dual GETHER (but only the GETHER had a TSU). Since the GETHER had 2 ports, and the same TSU is used for both, there is an option where as you put in CAM entries for multicast frame and such, you can select if you would like CAM entry packets received on one port to be automatically relayed over to the other port for processing (ie, bridge network). The POST1-POST4 registers are what you use to select what CAM entries you want relayed. For example: Register: CAM Entry Table POST1 Register (TSU_POST1) Bits: 31 to 28 Bit name: POST0[3:0] Description: These bits set the conditions for referring to CAM entry table 0. By setting multiple bits to 1, multiple conditions can be selected. POST0[3]: CAM entry table 0 is referred to in port 0 reception POST0[2]: CAM entry table 0 is referred to in port 0 to 1 relay. POST0[1]: CAM entry table 0 is referred to in port 1 reception. POST0[0]: CAM entry table 0 is referred to in port 1 to 0 relay So, as you can see, having the POST registers means nothing if you only have 1 Ethernet port attached to the TSU. Note, if you look at function sh_eth_tsu_get_post_bit, the relay functionality is never used anyway because each entry will only ever be set to "port 0 reception" or "port 1 reception". >> .shift_rd0 = 1, >> }; >> >> @@ -2460,6 +2461,9 @@ static void sh_eth_tsu_enable_cam_entry_post(struct >> net_device *ndev, >> u32 tmp; >> void *reg_offset; >> >> +if (mdp->cd->tsu_no_post) >> +return; >> + >> reg_offset = sh_eth_tsu_get_post_reg_offset(mdp, entry); > >I'd check check for SH_ETH_OFFSET_INVALID in the above function and return > NULL if so; then > we can check for NULL here... That was similar to my first fix. But, then I got to thinking that if a new RZ comes out with dual Ethernet, the designers might add in the POST registers back in the TSU. And in that case, it would be nice to reuse the sh_eth_is_rz_fast_ether array (just add in the extra registers). But...maybe checking for SH_ETH_OFFSET_INVALID is good for now instead of worrying about that. > Oh, and I'll have to correct your language and terminology. :-/ Should be > "if they don't exist" in the subject. Ya, my first subject line was too long, so I kept trying to shorten itand then it turn into bad grammer. I think a more clear subject is "Fix TSU without relay feature accesses" Chris
[PATCH] net: ethernet: renesas: sh_eth: do not access POST registers if not exist
The RZ/A1 has a TSU, but since it only has one Ethernet port, it does not
have POST registers. Therefore, if you try to write to register index
TSU_POST1 (which will be because it does not exist), it will either
panic or corrupt memory elsewhere.
Reported-by: Daniel Palmer <dan...@0x0f.com>
Signed-off-by: Chris Brandt <chris.bra...@renesas.com>
---
drivers/net/ethernet/renesas/sh_eth.c | 7 +++
drivers/net/ethernet/renesas/sh_eth.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/renesas/sh_eth.c
b/drivers/net/ethernet/renesas/sh_eth.c
index 1f8240a..850a13c 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -532,6 +532,7 @@ static struct sh_eth_cpu_data r7s72100_data = {
.no_ade = 1,
.hw_crc = 1,
.tsu= 1,
+ .tsu_no_post= 1,
.shift_rd0 = 1,
};
@@ -2460,6 +2461,9 @@ static void sh_eth_tsu_enable_cam_entry_post(struct
net_device *ndev,
u32 tmp;
void *reg_offset;
+ if (mdp->cd->tsu_no_post)
+ return;
+
reg_offset = sh_eth_tsu_get_post_reg_offset(mdp, entry);
tmp = ioread32(reg_offset);
iowrite32(tmp | sh_eth_tsu_get_post_bit(mdp, entry), reg_offset);
@@ -2472,6 +2476,9 @@ static bool sh_eth_tsu_disable_cam_entry_post(struct
net_device *ndev,
u32 post_mask, ref_mask, tmp;
void *reg_offset;
+ if (mdp->cd->tsu_no_post)
+ return false;
+
reg_offset = sh_eth_tsu_get_post_reg_offset(mdp, entry);
post_mask = sh_eth_tsu_get_post_mask(entry);
ref_mask = sh_eth_tsu_get_post_bit(mdp, entry) & ~post_mask;
diff --git a/drivers/net/ethernet/renesas/sh_eth.h
b/drivers/net/ethernet/renesas/sh_eth.h
index d050f37..ae34f2e 100644
--- a/drivers/net/ethernet/renesas/sh_eth.h
+++ b/drivers/net/ethernet/renesas/sh_eth.h
@@ -484,6 +484,7 @@ struct sh_eth_cpu_data {
unsigned tpauser:1; /* EtherC have TPAUSER */
unsigned bculr:1; /* EtherC have BCULR */
unsigned tsu:1; /* EtherC have TSU */
+ unsigned tsu_no_post:1; /* EtherC have TSU, but no POST */
unsigned hw_swap:1; /* E-DMAC have DE bit in EDMR */
unsigned rpadir:1; /* E-DMAC have RPADIR */
unsigned no_trimd:1;/* E-DMAC DO NOT have TRIMD */
--
2.9.2
