On 02/26/2015 11:13 PM, Ben Hutchings wrote:

quoted

quoted

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).

quoted

     Well, if your testing does match the manual, the reverted patch was most
probably just wrong in the first place.

quoted

quoted

Signed-off-by: Ben Hutchings <redacted>
---
   drivers/net/ethernet/renesas/sh_eth.c |    5 ++---
   1 file changed, 2 insertions(+), 3 deletions(-)

quoted

quoted

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

[...]

quoted

@@ -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

quoted

     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...

quoted

quoted

   		 * driver needs right shifting by 16.
   		 */
   		if (mdp->cd->shift_rd0)

quoted

     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