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

[michael.firth at bt.com]

> -----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?

Thanks

Michael