netdev - Re: [PATCH v5 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: <52D70B03.9040506@cogentembedded.com>
Date:	Thu, 16 Jan 2014 01:26:11 +0300
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	Simon Horman <horms+renesas@...ge.net.au>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	linux-sh@...r.kernel.org
CC:	linux-arm-kernel@...ts.infradead.org,
	Magnus Damm <magnus.damm@...il.com>
Subject: Re: [PATCH v5 net-next 2/4] sh_eth: Add support for r7s72100

Hello.

On 01/15/2014 09:12 AM, Simon Horman wrote:

> This is a fast ethernet controller.

    I have to say it's not exact enough patch description: R7S72100 is not 
Ethernet controller itself, it's a SoC containing the Ethernet controller.

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

> ---
> v5
> * As suggested by Sergei Shtylyov
>    - Do not use sh_eth_chip_reset_r8a7740 as it accesses non-existent
>      RMII registers. Instead use sh_eth_chip_reset.
>    - Do not use sh_eth_set_rate_gether as it accesses non-existent registers.
>    - Do not use reserved LCHNG bit of ECSR
>    - Do not use reserved LCHNGIP bit of ECSIPR
>    - Document that R8A779x also needs a 16 bit shift of the RFS bits
>    - Do not document that the R7S72100 has GECMR, it does not

   The above change list was moved from v2 section and doesn't match the real 
changes done in v5. ;-)

> v4
> * As requested by David Miller
>    - Use a boolean for the return value of sh_eth_is_rz_fast_ether()
>    - Correct coding style in sh_eth_get_stats()

> v3
> * No change

> v2
> * As suggested by Magnus Damm and Sergei Shtylyov
>    - r7s72100 ethernet is not gigabit so do not refer to it as such

> * As suggested by Magnus Damm
>    - As RZ specific register layout rather than using the gigabit layout
>      which includes registers that do not exist on this chip.
> ---
>   drivers/net/ethernet/renesas/sh_eth.c | 126 ++++++++++++++++++++++++++++++++--
>   drivers/net/ethernet/renesas/sh_eth.h |   3 +-
>   2 files changed, 121 insertions(+), 8 deletions(-)

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 4f5cfad..a7a0555 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -190,6 +190,64 @@ static const u16 sh_eth_offset_fast_rcar[SH_ETH_MAX_REGISTER_OFFSET] = {
>   	[TRIMD]		= 0x027c,
>   };
>
> +static const u16 sh_eth_offset_fast_rz[SH_ETH_MAX_REGISTER_OFFSET] = {

    Shouldn't this map precede R-Car one since this SoC is newer the same way 
you've reordered *enum* values, etc.? Sorry for not noticing in the previous 
review...

[...]
> +	[ARSTR]		= 0x0000,
> +	[TSU_CTRST]	= 0x0004,
> +	[TSU_VTAG0]	= 0x0058,
> +	[TSU_ADSBSY]	= 0x0060,
> +	[TSU_TEN]	= 0x0064,
> +	[TXNLCR0]	= 0x0080,
> +	[TXALCR0]	= 0x0084,
> +	[RXNLCR0]	= 0x0088,
> +	[RXALCR0]	= 0x008C,

    Well, the above counter register subgroup stands out from the TSU_* 
registers in the Gigabit mapping, not sure if we should follow that. These 
registers are not currently used anyway...

> +	[TSU_ADRH0]	= 0x0100,
> +	[TSU_ADRL0]	= 0x0104,
> +	[TSU_ADRH31]	= 0x01f8,
> +	[TSU_ADRL31]	= 0x01fc,
> +};
> +
>   static const u16 sh_eth_offset_fast_sh4[SH_ETH_MAX_REGISTER_OFFSET] = {
>   	[ECMR]		= 0x0100,
>   	[RFLR]		= 0x0108,
> @@ -318,6 +376,14 @@ static bool sh_eth_is_gether(struct sh_eth_private *mdp)
>   		return false;
>   }
>
> +static bool sh_eth_is_rz_fast_ether(struct sh_eth_private *mdp)
> +{
> +	if (mdp->reg_offset == sh_eth_offset_fast_rz)
> +		return true;
> +	else
> +		return false;

    Perhaps you should compress the above functions to one-liners as Joe has 
suggested. Or I/you could do it in a separate patch...

> +}
> +
>   static void sh_eth_select_mii(struct net_device *ndev)
>   {
>   	u32 value = 0x0;
[...]
> @@ -1309,9 +1409,9 @@ 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's GETHER, the RFS
> -		 * bits are from bit 25 to bit 16. So, the driver needs right
> -		 * shifting by 16.
> +		 * bit 0. However, in case of the R8A7740, R8A779x and

    Small nit: comma needed before "and" as far as I know English grammar.

> +		 * R7S72100 the RFS bits are from bit 25 to bit 16. So, the
> +		 * driver needs right shifting by 16.
>   		 */
>   		if (mdp->cd->shift_rd0)
>   			desc_status >>= 16;

    Other than that, this looks fine now, you can add my:

Acked-by: Sergei Shtylyov <sergei.shtylyov@...entembedded.com>

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