Re: [PATCH v2] net/sh-eth: Add support selecting MII function for SH7734 and R8A7740

[Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>]

Hi,

Thank you for your review.

Florian Fainelli さんは書きました:
> On Tuesday 12 June 2012 16:29:02 Nobuhiro Iwamatsu wrote:
>> Ethernet IP of SH7734 and R8A7740 has selecting MII register.
>> The user needs to change a value according to MII to be used.
>> This adds the function to change the value of this register.
>>
>> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
>> ---
>>  V2: Fix the check by select_mii.
>>  drivers/net/ethernet/renesas/sh_eth.c |  106 
> ++++++++++++++++++++-------------
>>  drivers/net/ethernet/renesas/sh_eth.h |    1 +
>>  2 files changed, 65 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c 
> b/drivers/net/ethernet/renesas/sh_eth.c
>> index be3c221..5358804 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> @@ -49,6 +49,33 @@
>>  		NETIF_MSG_RX_ERR| \
>>  		NETIF_MSG_TX_ERR)
>>  
>> +#if defined(CONFIG_CPU_SUBTYPE_SH7734) || defined(CONFIG_CPU_SUBTYPE_SH7763) 
> || \
>> +	defined(CONFIG_ARCH_R8A7740)
>> +static void sh_eth_select_mii(struct net_device *ndev)
>> +{
>> +	u32 value = 0x0;
>> +	struct sh_eth_private *mdp = netdev_priv(ndev);
>> +
>> +	switch (mdp->phy_interface) {
>> +	case PHY_INTERFACE_MODE_GMII:
>> +		value = 0x2;
>> +		break;
>> +	case PHY_INTERFACE_MODE_MII:
>> +		value = 0x1;
>> +		break;
>> +	case PHY_INTERFACE_MODE_RMII:
>> +		value = 0x0;
>> +		break;
>> +	default:
>> +		pr_warn("PHY interface mode was not setup. Set to MII.\n");
>> +		value = 0x1;
>> +		break;
>> +	}
>> +
>> +	sh_eth_write(ndev, value, RMII_MII);
>> +}
>> +#endif
>> +
>>  /* There is CPU dependent code */
>>  #if defined(CONFIG_CPU_SUBTYPE_SH7724)
>>  #define SH_ETH_RESET_DEFAULT	1
>> @@ -283,6 +310,7 @@ static struct sh_eth_cpu_data 
> *sh_eth_get_cpu_data(struct sh_eth_private *mdp)
>>  #elif defined(CONFIG_CPU_SUBTYPE_SH7734) || 
> defined(CONFIG_CPU_SUBTYPE_SH7763)
>>  #define SH_ETH_HAS_TSU	1
>>  static void sh_eth_reset_hw_crc(struct net_device *ndev);
>> +
>>  static void sh_eth_chip_reset(struct net_device *ndev)
>>  {
>>  	struct sh_eth_private *mdp = netdev_priv(ndev);
>> @@ -292,35 +320,6 @@ static void sh_eth_chip_reset(struct net_device *ndev)
>>  	mdelay(1);
>>  }
>>  
>> -static void sh_eth_reset(struct net_device *ndev)
>> -{
>> -	int cnt = 100;
>> -
>> -	sh_eth_write(ndev, EDSR_ENALL, EDSR);
>> -	sh_eth_write(ndev, sh_eth_read(ndev, EDMR) | EDMR_SRST_GETHER, EDMR);
>> -	while (cnt > 0) {
>> -		if (!(sh_eth_read(ndev, EDMR) & 0x3))
>> -			break;
>> -		mdelay(1);
>> -		cnt--;
>> -	}
>> -	if (cnt == 0)
>> -		printk(KERN_ERR "Device reset fail\n");
>> -
>> -	/* Table Init */
>> -	sh_eth_write(ndev, 0x0, TDLAR);
>> -	sh_eth_write(ndev, 0x0, TDFAR);
>> -	sh_eth_write(ndev, 0x0, TDFXR);
>> -	sh_eth_write(ndev, 0x0, TDFFR);
>> -	sh_eth_write(ndev, 0x0, RDLAR);
>> -	sh_eth_write(ndev, 0x0, RDFAR);
>> -	sh_eth_write(ndev, 0x0, RDFXR);
>> -	sh_eth_write(ndev, 0x0, RDFFR);
>> -
>> -	/* Reset HW CRC register */
>> -	sh_eth_reset_hw_crc(ndev);
>> -}
>> -
>>  static void sh_eth_set_duplex(struct net_device *ndev)
>>  {
>>  	struct sh_eth_private *mdp = netdev_priv(ndev);
>> @@ -377,9 +376,43 @@ static struct sh_eth_cpu_data sh_eth_my_cpu_data = {
>>  	.tsu		= 1,
>>  #if defined(CONFIG_CPU_SUBTYPE_SH7734)
>>  	.hw_crc     = 1,
>> +	.select_mii = 1,
>>  #endif
>>  };
>>  
>> +static void sh_eth_reset(struct net_device *ndev)
>> +{
>> +	int cnt = 100;
>> +
>> +	sh_eth_write(ndev, EDSR_ENALL, EDSR);
>> +	sh_eth_write(ndev, sh_eth_read(ndev, EDMR) | EDMR_SRST_GETHER, EDMR);
>> +	while (cnt > 0) {
>> +		if (!(sh_eth_read(ndev, EDMR) & 0x3))
>> +			break;
>> +		mdelay(1);
>> +		cnt--;
>> +	}
>> +	if (cnt == 0)
>> +		printk(KERN_ERR "Device reset fail\n");
> 
> It looks like this would need a subsequent fix. Failing to reset the adapter 
> and just erroring out and not returning an error looks obviously wrong. Since 
> sh_eth_reset() is called in sh_eth_dev_init() which does return an int, 
> propagate the error back to the caller.

Yes, you are right. I will fix your point and send a patch.

Best regards,
   Nobuhiro