Re: [PATCH net 3/4] Revert "sh_eth: Enable Rx descriptor word 0 shift for r8a7790"

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

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