netdev - Re: [PATCH v4 net-next 2/4] sh_eth: Add support for r7s72100

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52D7FC8D.7000800@cogentembedded.com>
Date:	Thu, 16 Jan 2014 19:36:45 +0400
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	Simon Horman <horms@...ge.net.au>
CC:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	linux-sh@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Magnus Damm <magnus.damm@...il.com>
Subject: Re: [PATCH v4 net-next 2/4] sh_eth: Add support for r7s72100

Hello.

On 16-01-2014 4:49, Simon Horman wrote:

>>>>> This is a fast ethernet controller.

>>>>> Signed-off-by: Simon Horman <horms+renesas@...ge.net.au>

>>>> [...]

>>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
>>>>> index 4b38533..cc6d4af 100644
>>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>>>> @@ -190,6 +190,59 @@ static const u16 sh_eth_offset_fast_rcar[SH_ETH_MAX_REGISTER_OFFSET] = {
>> [...]
>>>>> @@ -701,6 +762,35 @@ static struct sh_eth_cpu_data r8a7740_data = {
>>>>>   	.shift_rd0	= 1,
>>>>>   };
>>>>>
>>>>> +/* R7S72100 */
>>>>> +static struct sh_eth_cpu_data r7s72100_data = {
>>>>> +	.chip_reset	= sh_eth_chip_reset,
>>>>> +	.set_duplex	= sh_eth_set_duplex,
>>>>> +
>>>>> +	.register_type	= SH_ETH_REG_FAST_RZ,
>>>>> +
>>>>> +	.ecsr_value	= ECSR_ICD,
>>>>> +	.ecsipr_value	= ECSIPR_ICDIP,
>>>>> +	.eesipr_value	= 0xff7f009f,
>>>>> +
>>>>> +	.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,
>>>>> +	.fdr_value	= 0x0000070f,
>>>>> +	.rmcr_value	= RMCR_RNC,
>>>>> +
>>>>> +	.apr		= 1,
>>>>> +	.mpr		= 1,
>>>>> +	.tpauser	= 1,
>>>>> +	.hw_swap	= 1,
>>>>> +	.rpadir		= 1,
>>>>> +	.rpadir_value   = 2 << 16,
>>>>> +	.no_trimd	= 1,
>>>>> +	.tsu		= 1,
>>>>> +	.shift_rd0	= 1,
>>
>>>>     Perhaps this field should be renamed to something talking about
>>>> check summing support (since bits 0..15 of RD0 contain a frame check
>>>> sum for those SoCs). Or maybe it should be just merged with the
>>>> 'hw_crc' field...

>>> I have no feelings about that one way or another.

>>     Do you happen to have R8A7740 manual by chance? If so, does it
>> talk about RX check summing support and using RD0 for that?

> Yes and yes.

> I have taken a quick look and the documentation for RX checksumming on the
> R8A7740 appears to be very similar if not the same as that of the R7S72100.

> In particular both refer to using the bottom 16 bits of RD0 as
> containing the packet checksum.

    OK, now if you had SH7734 manual to completely confirm that check sum is 
stored in the same place there... most probably it is, of course, and we 
should merge 'hw_crc' and 'shift_rd0' into a single field.

[...]
>>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
>>>>> index 0fe35b7..0bcde90 100644
>>>>> --- a/drivers/net/ethernet/renesas/sh_eth.h
>>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.h
>> [...]
>>>>> @@ -191,6 +192,7 @@ enum DMAC_M_BIT {
>>>>>   /* EDTRR */
>>>>>   enum DMAC_T_BIT {
>>>>>   	EDTRR_TRNS_GETHER = 0x03,
>>>>> +	EDTRR_TRNS_RZ_ETHER = 0x03,

>>>>     I doubt we need a special case here. You didn't introduce one for
>>>> the software reset bits.

>>> True, but RZ is not Gigabit. So I think we either need two names
>>> or to choose a more generic name.

>>     Well, R7S72100 manual calls these bits just TR[1:0]. Don't know
>> what SoCs having Gigabit call it in the manuals...

>>>>>   	EDTRR_TRNS_ETHER = 0x01,

>>     R-Car manuals seem to call the bit TRNS (as well as the
>> prehistoric SH manuals probably). Perhaps we could use that
>> difference, TRNS vs TR, don't know...

> Perhaps we should just leave it as-is, using EDTRR_TRNS_GETHER and
> EDTRR_TRNS_RZ_ETHER, after all.

    No, I liked your last version more. At least it's more consistent, not 
adding separate values for either TR[1:0] or soft reset bits.

> At least until we can think of a better names :)

    I doubt we can come up with something better.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html