| Message ID | 20160826200107.20681-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hello. On 08/26/2016 11:01 PM, Chris Brandt 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? :-) > Therefore, if you try to write to register index > TSU_POST1 (which will be FFFF because it does not exist), it will either > panic or corrupt memory elsewhere. The true reason of that is that Ben Hutchings wasn't consistent with handling of SH_ETH_OFFSET_INVALID: he didn't add WARN_ON() to sh_eth_tsu_{read|wrte}() and friends. Maybe you can do this? > Reported-by: Daniel Palmer <daniel@0x0f.com> > Signed-off-by: Chris Brandt <chris.brandt@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, The rest of the code seems to use sh_eth_is_rz_fast_ether() to differentiate the limited TSU implementation in the RZ/A1 SoC -- see sh_eth_tsu_init(). I'd prefer if you follow this suit. Either that or give this bitfield a different name. > .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... > 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); ... and here. > post_mask = sh_eth_tsu_get_post_mask(entry); > ref_mask = sh_eth_tsu_get_post_bit(mdp, entry) & ~post_mask; [...] MBR, Sergei
Hello.
Oh, and I'll have to correct your language and terminology. :-/
Should be "if they don't exist" in the subject.
On 08/26/2016 11:01 PM, Chris Brandt wrote:
> 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 FFFF because it does not exist),
It's not a register index which is 0xFFFF but the register offset (fetched
from a layout table using the index).
> it will either panic or corrupt memory elsewhere.
>
> Reported-by: Daniel Palmer <daniel@0x0f.com>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
[...]
MBR, Sergei
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 it....and then it turn into bad grammer. I think a more clear subject is "Fix TSU without relay feature accesses" Chris
Hello. On 08/29/2016 05:41 PM, Chris Brandt 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. Good to have someone with the old SH manuals. :-) > 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. SH7757 is not the only platform with TSU, there's e.g. R8A7740 ARM SoC which only has 1 GETHER channel... > 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. That probably means we have more issues on other SoCs having 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". Yes, seeing this... >>> .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... I hadn't thought it thru it seems... I meant that the check includes WARN_ON() but we generally should avoid hitting this code, so another check is needed beforehand, like that one you added... > 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). It seems having something like 'dual_ch' flag would fit... > But...maybe checking for SH_ETH_OFFSET_INVALID is good for now instead of worrying about that. We just need 2 patches, I think... >> 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, For what? > so I kept trying to shorten it....and then it turn into bad grammer. > > I think a more clear subject is "Fix TSU without relay feature accesses" Or a "single channel TSU". > Chris MBR, Sergei
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
Hi Chris, On Tue, Aug 30, 2016 at 4:16 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > 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. R8a7740 does have the POST registers. > 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. 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
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
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 */
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 FFFF because it does not exist), it will either panic or corrupt memory elsewhere. Reported-by: Daniel Palmer <daniel@0x0f.com> Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/net/ethernet/renesas/sh_eth.c | 7 +++++++ drivers/net/ethernet/renesas/sh_eth.h | 1 + 2 files changed, 8 insertions(+)