[U-Boot-Users] TSEC Ethernet driver patch - RFC

[Ben Warren]

michael.firth at bt.com wrote:
>> -----Original Message-----
>> From: u-boot-users-bounces at lists.sourceforge.net 
>> [mailto:u-boot-users-bounces at lists.sourceforge.net] On Behalf 
>> Of michael.firth at bt.com
>> Sent: 09 January 2008 20:26
>> To: biggerbadderben at gmail.com
>> Cc: u-boot-users at lists.sourceforge.net
>> Subject: Re: [U-Boot-Users] TSEC Ethernet driver patch - RFC
>>
>>
>>     
>>> -----Original Message-----
>>> From: Ben Warren [mailto:biggerbadderben at gmail.com]
>>> Sent: 08 January 2008 16:42
>>> To: Firth,MJC,Michael,DMM R
>>> Cc: u-boot-users at lists.sourceforge.net
>>> Subject: Re: [U-Boot-Users] Possible TSEC Ethernet driver patch
>>>
>>> michael.firth at bt.com wrote:
>>>       
>>>> While debugging a board recently I found that the MDIO
>>>>         
>>> (mii) command
>>>       
>>>> support in the TSEC Ethernet driver is somewhat unhelpful.
>>>>
>>>> Currently, even though there is a comment in the code that
>>>>         
>>> "For now,
>>>       
>>>> only TSEC1 (index 0) has access to the PHYs, so all of 
>>>>         
>> the entries 
>>     
>>>> have '0'", all MDIO commands are processed by searching 
>>>>         
>> for a TSEC 
>>     
>>>> instance that has the requested MDIO address associated
>>>>         
>>> with it, and
>>>       
>>>> then using that instance to run the command, even though,
>>>>         
>>> because of
>>>       
>>>> the aforementioned comment, all instances process MDIO commands 
>>>> through the same port.
>>>>
>>>> This means that it is only possible to communicate with
>>>>         
>>> MDIO addresses
>>>       
>>>> that have a TSEC instance associated with them, even though the 
>>>> hardware is capable of accessing any address. It also means
>>>>         
>>> that there
>>>       
>>>> is a list search that isn't needed.
>>>>
>>>> I have patched the 1.3.1 U-Boot code to remove this 
>>>>         
>> search, and to 
>>     
>>>> interrogate the requested PHY directly. This means that it
>>>>         
>>> is possible
>>>       
>>>> to directly access all 32 PHY addresses.
>>>>
>>>> Is this a change that would be more generally useful to 
>>>>         
>> the U-Boot 
>>     
>>>> community, and, if so, how should I submit a more general 
>>>>         
>> patch for 
>>     
>>>> this?
>>>>
>>>>   
>>>>         
>>> Why don't you post what you have, clearly label it as 'RFC' 
>>> and we'll have a look. In my spare time (very spare indeed) 
>>>       
>> I'm trying 
>>     
>>> to decouple PHYs from MACs, but time is hard to find and meanwhile 
>>> things need to work.
>>>
>>> regards,
>>> Ben
>>>
>>>       
>> Patch below.
>>
>> The main area I wasn't sure on how to handle was how to 
>> replace the other calls to 'read_phy_reg' and 'write_phy_reg' 
>> in the code. Currently I've used a #define to map these on to 
>> the modified functions that take the phy address as a parameter.
>>
>> --- u-boot-1.3.1-orig/drivers/net/tsec.c	2007-12-06
>> 09:21:19.000000000 +0000
>> +++ u-boot-1.3.1/drivers/net/tsec.c	2008-01-09 20:19:36.000000000
>> +0000
>> @@ -241,10 +244,9 @@
>>   * It will wait for the write to be done (or for a timeout to
>>   * expire) before exiting
>>   */
>> -void write_phy_reg(struct tsec_private *priv, uint regnum, 
>> uint value)
>> +void write_any_phy_reg(struct tsec_private *priv, uint phyid, uint
>> regnum, uint value)
>>  {
>>  	volatile tsec_t *regbase = priv->phyregs;
>> -	uint phyid = priv->phyaddr;
>>  	int timeout = 1000000;
>>  
>>  	regbase->miimadd = (phyid << 8) | regnum; @@ -255,17 +257,19 @@
>>  	while ((regbase->miimind & MIIMIND_BUSY) && timeout--) ;  }
>>  
>> +/* #define to provide old write_phy_reg functionality without
>> duplicating code */
>> +#define write_phy_reg(priv, regnum, value)
>> write_any_phy_reg(priv,priv->phyaddr,regnum,value)
>> +
>>  /* Reads register regnum on the device's PHY through the
>>   * registers specified in priv.	 It lowers and raises the read
>>   * command, and waits for the data to become valid (miimind
>>   * notvalid bit cleared), and the bus to cease activity (miimind
>>   * busy bit cleared), and then returns the value
>>   */
>> -uint read_phy_reg(struct tsec_private *priv, uint regnum)
>> +uint read_any_phy_reg(struct tsec_private *priv, uint phyid, uint
>> regnum)
>>  {
>>  	uint value;
>>  	volatile tsec_t *regbase = priv->phyregs;
>> -	uint phyid = priv->phyaddr;
>>  
>>  	/* Put the address of the phy, and the register
>>  	 * number into MIIMADD */
>> @@ -288,6 +292,9 @@
>>  	return value;
>>  }
>>  
>> +/* #define to provide old read_phy_reg functionality without
>> duplicating code */
>> +#define read_phy_reg(priv,regnum)
>> read_any_phy_reg(priv,priv->phyaddr,regnum)
>> +
>>  /* Discover which PHY is attached to the device, and configure it
>>   * properly.  If the PHY is not recognized, then return 0
>>   * (failure).  Otherwise, return 1
>> @@ -1487,18 +1494,6 @@
>>  #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
>>  	&& !defined(BITBANGMII)
>>  
>> -struct tsec_private *get_priv_for_phy(unsigned char phyaddr) -{
>> -	int i;
>> -
>> -	for (i = 0; i < MAXCONTROLLERS; i++) {
>> -		if (privlist[i]->phyaddr == phyaddr)
>> -			return privlist[i];
>> -	}
>> -
>> -	return NULL;
>> -}
>> -
>>  /*
>>   * Read a MII PHY register.
>>   *
>> @@ -1509,14 +1504,14 @@
>>  			    unsigned char reg, unsigned short *value)  {
>>  	unsigned short ret;
>> -	struct tsec_private *priv = get_priv_for_phy(addr);
>> +	struct tsec_private *priv = privlist[0];
>>  
>>  	if (NULL == priv) {
>>  		printf("Can't read PHY at address %d\n", addr);
>>  		return -1;
>>  	}
>>  
>> -	ret = (unsigned short)read_phy_reg(priv, reg);
>> +	ret = (unsigned short)read_any_phy_reg(priv, addr, reg);
>>  	*value = ret;
>>  
>>  	return 0;
>> @@ -1531,14 +1526,14 @@
>>  static int tsec_miiphy_write(char *devname, unsigned char addr,
>>  			     unsigned char reg, unsigned short value)  {
>> -	struct tsec_private *priv = get_priv_for_phy(addr);
>> +	struct tsec_private *priv = privlist[0];
>>  
>>  	if (NULL == priv) {
>>  		printf("Can't write PHY at address %d\n", addr);
>>  		return -1;
>>  	}
>>  
>> -	write_phy_reg(priv, reg, value);
>> +	write_any_phy_reg(priv, addr, reg, value);
>>  
>>  	return 0;
>>  }
>>
>> Signed-off-by: Michael Firth <michael.firth at bt.com>
>>
>>     
> Does the lack of comments on this mean that people are happy with it, 
> or that they haven't had a chance to consider it yet?
>
>   
More the latter than the former, I'm afraid.  I'll hopefully have a look 
today.

regards,
Ben