Message ID | 1424960474.4444.21.camel@xylophone.i.decadent.org.uk |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, 2015-02-26 at 14:21 +0000, Ben Hutchings wrote: > This reverts commit fd9af07c3404ac9ecbd0d859563360f51ce1ffde. > > The hardware manual states that the frame error and multicast bits are > copied to bits 9:0 of RD0, not bits 25:16. I've tested that this is > true for RFS1 (CRC error), RFS3 (frame too short), RFS4 (frame too > long) and RFS8 (multicast). [...] This behaviour might of course be dependent on the revision of the chip. I didn't find a chip ID register in the manual, but I found the recent patch "ARM: shmobile: Add function to get SoCs revision data for R-Car Gen2". With that patch, /proc/cpuinfo on my test system shows: Hardware : Generic R8A7790 (Flattened Device Tree) Revision : 0020 Serial : 0000000000000000 Ben. -- 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 02/26/2015 05:21 PM, Ben Hutchings wrote: > This reverts commit fd9af07c3404ac9ecbd0d859563360f51ce1ffde. > The hardware manual states that the frame error and multicast bits are > copied to bits 9:0 of RD0, not bits 25:16. I've tested that this is > true for RFS1 (CRC error), RFS3 (frame too short), RFS4 (frame too > long) and RFS8 (multicast). Well, if your testing does match the manual, the reverted patch was most probably just wrong in the first place. > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > --- > drivers/net/ethernet/renesas/sh_eth.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > index ed67951f5271..317722e16043 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c [...] > @@ -1459,8 +1458,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota) > > /* In case of almost all GETHER/ETHERs, the Receive Frame State > * (RFS) bits in the Receive Descriptor 0 are from bit 9 to > - * bit 0. However, in case of the R8A7740, R8A779x, and > - * R7S72100 the RFS bits are from bit 25 to bit 16. So, the > + * bit 0. However, in case of the R8A7740 and R7S72100 > + * the RFS bits are from bit 25 to bit 16. So, the And that seems more logical to me, as we have the RFS bits starting with bit 16 only on the SoCs with the GEther compatible register layout (though the latter SoC doesn't support Gigabit speed). Having the RFS bits start at bit 16 is most probably connected to a SoC having support for hardware checksumming (bit 0-15 store the received frame checksum for at least R7S72100), so merging the 'shift_rd0' and 'hw_crc' flags seemed the reasonable next step to me (not taken due to the lack of documentation)... > * driver needs right shifting by 16. > */ > if (mdp->cd->shift_rd0) This hunk (inverted) was not a part of the commit you're reverting. Perhaps you shouldn't call this patch revert? Or make a remark about this hunk in the change-log? 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
On Thu, 2015-02-26 at 22:17 +0300, Sergei Shtylyov wrote: > Hello. > > On 02/26/2015 05:21 PM, Ben Hutchings wrote: > > > This reverts commit fd9af07c3404ac9ecbd0d859563360f51ce1ffde. > > > The hardware manual states that the frame error and multicast bits are > > copied to bits 9:0 of RD0, not bits 25:16. I've tested that this is > > true for RFS1 (CRC error), RFS3 (frame too short), RFS4 (frame too > > long) and RFS8 (multicast). > > Well, if your testing does match the manual, the reverted patch was most > probably just wrong in the first place. > > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > > --- > > drivers/net/ethernet/renesas/sh_eth.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > > index ed67951f5271..317722e16043 100644 > > --- a/drivers/net/ethernet/renesas/sh_eth.c > > +++ b/drivers/net/ethernet/renesas/sh_eth.c > [...] > > @@ -1459,8 +1458,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota) > > > > /* In case of almost all GETHER/ETHERs, the Receive Frame State > > * (RFS) bits in the Receive Descriptor 0 are from bit 9 to > > - * bit 0. However, in case of the R8A7740, R8A779x, and > > - * R7S72100 the RFS bits are from bit 25 to bit 16. So, the > > + * bit 0. However, in case of the R8A7740 and R7S72100 > > + * the RFS bits are from bit 25 to bit 16. So, the > > And that seems more logical to me, as we have the RFS bits starting with > bit 16 only on the SoCs with the GEther compatible register layout (though the > latter SoC doesn't support Gigabit speed). > Having the RFS bits start at bit 16 is most probably connected to a SoC > having support for hardware checksumming (bit 0-15 store the received frame > checksum for at least R7S72100), so merging the 'shift_rd0' and 'hw_crc' flags > seemed the reasonable next step to me (not taken due to the lack of > documentation)... After this patch there will still be: /* SH7757(GETHERC) */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 0 /* SH7734 */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 1, .shift_rd0 = 0 /* SH7763 */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 0 /* R8A7740 */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 1 /* R7S72100 */ .register_type = SH_ETH_REG_FAST_RZ, .hw_crc = 1, .shift_rd0 = 1 Do you really think R7S72100 is the only one of these with the flags set correctly? Also, the frame CRC is 32 bits and is surely checked by all versions of the MAC. Is the 16-bit 'CRC' actually a full-frame IP-style checksum? Someone should make the driver actually use that where available. (Not me, I don't have one of those fancy checksumming chips.) > > * driver needs right shifting by 16. > > */ > > if (mdp->cd->shift_rd0) > > This hunk (inverted) was not a part of the commit you're reverting. > Perhaps you shouldn't call this patch revert? Or make a remark about this hunk > in the change-log? Well, it's logically a revert. I could mention that I'm also fixing a comment to match. Ben. -- 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/26/2015 11:13 PM, Ben Hutchings wrote: >>> The hardware manual states that the frame error and multicast bits are >>> copied to bits 9:0 of RD0, not bits 25:16. I've tested that this is >>> true for RFS1 (CRC error), RFS3 (frame too short), RFS4 (frame too >>> long) and RFS8 (multicast). >> Well, if your testing does match the manual, the reverted patch was most >> probably just wrong in the first place. >>> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> >>> --- >>> drivers/net/ethernet/renesas/sh_eth.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c >>> index ed67951f5271..317722e16043 100644 >>> --- a/drivers/net/ethernet/renesas/sh_eth.c >>> +++ b/drivers/net/ethernet/renesas/sh_eth.c >> [...] >>> @@ -1459,8 +1458,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota) >>> >>> /* In case of almost all GETHER/ETHERs, the Receive Frame State >>> * (RFS) bits in the Receive Descriptor 0 are from bit 9 to >>> - * bit 0. However, in case of the R8A7740, R8A779x, and >>> - * R7S72100 the RFS bits are from bit 25 to bit 16. So, the >>> + * bit 0. However, in case of the R8A7740 and R7S72100 >>> + * the RFS bits are from bit 25 to bit 16. So, the >> And that seems more logical to me, as we have the RFS bits starting with >> bit 16 only on the SoCs with the GEther compatible register layout (though the >> latter SoC doesn't support Gigabit speed). >> Having the RFS bits start at bit 16 is most probably connected to a SoC >> having support for hardware checksumming (bit 0-15 store the received frame >> checksum for at least R7S72100), so merging the 'shift_rd0' and 'hw_crc' flags >> seemed the reasonable next step to me (not taken due to the lack of >> documentation)... > After this patch there will still be: > /* SH7757(GETHERC) */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 0 > /* SH7734 */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 1, .shift_rd0 = 0 > /* SH7763 */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 0 > /* R8A7740 */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 1 > /* R7S72100 */ .register_type = SH_ETH_REG_FAST_RZ, .hw_crc = 1, .shift_rd0 = 1 > Do you really think R7S72100 is the only one of these with the flags set > correctly? I can't be certain since I only have R7S72100 manual but extrapolating it to other SoCs seemed reasonable enough. The driver itself doesn't support checksum offload, so the 'hw_crc' flag have little value currently, I think. > Also, the frame CRC is 32 bits and is surely checked by all versions of > the MAC. > Is the 16-bit 'CRC' actually a full-frame IP-style checksum? I didn't mean frame CRC, I did mean (probably) IP packet checksum (which is 16-bit indeed). The flag name seems just wrong. > Someone should make the driver actually use that where available. (Not > me, I don't have one of those fancy checksumming chips.) I don't (yet) have access to R7S72100 either, let alone the older SoCs... >>> * driver needs right shifting by 16. >>> */ >>> if (mdp->cd->shift_rd0) >> This hunk (inverted) was not a part of the commit you're reverting. >> Perhaps you shouldn't call this patch revert? Or make a remark about this hunk >> in the change-log? > Well, it's logically a revert. I could mention that I'm also fixing a > comment to match. Yes, please. > Ben. 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: Thu, 26 Feb 2015 23:35:12 +0300 > On 02/26/2015 11:13 PM, Ben Hutchings wrote: >> Well, it's logically a revert. I could mention that I'm also fixing a >> comment to match. > > Yes, please. Ok, then I'm waiting for a respin of this series. -- 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 ed67951f5271..317722e16043 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -508,7 +508,6 @@ static struct sh_eth_cpu_data r8a779x_data = { .tpauser = 1, .hw_swap = 1, .rmiimode = 1, - .shift_rd0 = 1, }; static void sh_eth_set_rate_sh7724(struct net_device *ndev) @@ -1459,8 +1458,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota) /* In case of almost all GETHER/ETHERs, the Receive Frame State * (RFS) bits in the Receive Descriptor 0 are from bit 9 to - * bit 0. However, in case of the R8A7740, R8A779x, and - * R7S72100 the RFS bits are from bit 25 to bit 16. So, the + * bit 0. However, in case of the R8A7740 and R7S72100 + * the RFS bits are from bit 25 to bit 16. So, the * driver needs right shifting by 16. */ if (mdp->cd->shift_rd0)
This reverts commit fd9af07c3404ac9ecbd0d859563360f51ce1ffde. The hardware manual states that the frame error and multicast bits are copied to bits 9:0 of RD0, not bits 25:16. I've tested that this is true for RFS1 (CRC error), RFS3 (frame too short), RFS4 (frame too long) and RFS8 (multicast). Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> --- drivers/net/ethernet/renesas/sh_eth.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)