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

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

[michael.firth at bt.com]

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?

Michael

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

[Ben Warren]

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

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

[David Saada]

Michael,
This would certainly be useful. In fact I thought of doing so myself as it
bothered my as well...
Furthermore, I submitted a patch couple of days ago, which added (among the
rest) MII command support to the UEC driver (basically a copy from the TSEC
code). I guess this kind of patch should go there too.
If you plan to submit this patch please inform - otherwise I'll do it myself
(I think its quite a simple one).
Regards,
David.


michael.firth 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?
> 
> Michael
> 
> -------------------------------------------------------------------------
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services for
> just about anything Open Source.
> http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users
> 
> 

-- 
View this message in context: http://www.nabble.com/Possible-TSEC-Ethernet-driver-patch-tp14692943p14693344.html
Sent from the Uboot - Users mailing list archive at Nabble.com.

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

[michael.firth at bt.com]

> -----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@bt.com>

[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@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

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

[David Saada]

> 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
Thumbs up on my end. Was going to implement it the same way...
David.

[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@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

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

[Andy Fleming]

On Jan 9, 2008 2:26 PM,  <michael.firth@bt.com> wrote:
>

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

So, for the most part, I'm happy with this change.  I suspect that I
over-engineered it, originally, anticipating the possibility that a
future part might make use of the other mdio interfaces.

The biggest problem I see is that the TBI PHYs, which are internal to
each TSEC, are accessed through the other mdio interfaces.  Right now
this isn't really supported, but there's a desire to expose these,
since they are used for SGMII configuration.  I hadn't yet figured out
the best way to do that, but this change would potentially make it
more difficult.

Ideally, we would stop referring to PHYs only by address on the bus.
There could be multiple busses (and, in fact, there are), so the ideal
solution would deal with that.  But that's a hefty task (which I'm
hoping Ben finds time for), so I'm not really suggesting that for the
short term.  For now it's probably fine as long as it doesn't make
Ben's job harder.  But it's not really changing the higher-level
interface, so that should be ok, too.

Andy

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

[michael.firth at bt.com]

> -----Original Message-----
> From: Andy Fleming [mailto:afleming at gmail.com] 
> Sent: 15 January 2008 16:56
> To: Firth,MJC,Michael,DMJ R
> Cc: biggerbadderben at gmail.com; u-boot-users at lists.sourceforge.net
> Subject: Re: [U-Boot-Users] TSEC Ethernet driver patch - RFC
> 
> On Jan 9, 2008 2:26 PM,  <michael.firth@bt.com> wrote:
> >
> 
> So, for the most part, I'm happy with this change.  I suspect 
> that I over-engineered it, originally, anticipating the 
> possibility that a future part might make use of the other 
> mdio interfaces.
> 
> The biggest problem I see is that the TBI PHYs, which are 
> internal to each TSEC, are accessed through the other mdio 
> interfaces.  Right now this isn't really supported, but 
> there's a desire to expose these, since they are used for 
> SGMII configuration.  I hadn't yet figured out the best way 
> to do that, but this change would potentially make it more difficult.
> 
> Ideally, we would stop referring to PHYs only by address on the bus.
> There could be multiple busses (and, in fact, there are), so 
> the ideal solution would deal with that.  But that's a hefty 
> task (which I'm hoping Ben finds time for), so I'm not really 
> suggesting that for the short term.  For now it's probably 
> fine as long as it doesn't make Ben's job harder.  But it's 
> not really changing the higher-level interface, so that 
> should be ok, too.
> 
I guess a middle option is to make the two options (multi MDIO bus
support versus 
the ability to access all devices on a single MDIO bus) available via a
configuration option.

I think the way to do this from my patch is to instead retain the
'get_priv_for_phy' function 
within a #ifdef for the configuration option, and select whether to call
the function or always 
use the first TSEC instance, again based on the #define.

Given that, as you said, several people, have suggested that the whole
PHY support in U-Boot 
needs an overhaul, another question is whether any support for the
internal PHYs is likely to 
be implemented before this overhaul.

I guess that if the old functionality is still available within the
code, then, when the TBI 
support is implemented then the person that does that has easy access to
all the code options.

Regards

Michael

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

[Ben Warren]

Hi Michael,

michael.firth at bt.com wrote:
>> -----Original Message-----
>> From: Andy Fleming [mailto:afleming at gmail.com] 
>> Sent: 15 January 2008 16:56
>> To: Firth,MJC,Michael,DMJ R
>> Cc: biggerbadderben at gmail.com; u-boot-users at lists.sourceforge.net
>> Subject: Re: [U-Boot-Users] TSEC Ethernet driver patch - RFC
>>
>> On Jan 9, 2008 2:26 PM,  <michael.firth@bt.com> wrote:
>>     
>> So, for the most part, I'm happy with this change.  I suspect 
>> that I over-engineered it, originally, anticipating the 
>> possibility that a future part might make use of the other 
>> mdio interfaces.
>>
>> The biggest problem I see is that the TBI PHYs, which are 
>> internal to each TSEC, are accessed through the other mdio 
>> interfaces.  Right now this isn't really supported, but 
>> there's a desire to expose these, since they are used for 
>> SGMII configuration.  I hadn't yet figured out the best way 
>> to do that, but this change would potentially make it more difficult.
>>
>> Ideally, we would stop referring to PHYs only by address on the bus.
>> There could be multiple busses (and, in fact, there are), so 
>> the ideal solution would deal with that.  But that's a hefty 
>> task (which I'm hoping Ben finds time for), so I'm not really 
>> suggesting that for the short term.  For now it's probably 
>> fine as long as it doesn't make Ben's job harder.  But it's 
>> not really changing the higher-level interface, so that 
>> should be ok, too.
>>
>>     
> I guess a middle option is to make the two options (multi MDIO bus
> support versus 
> the ability to access all devices on a single MDIO bus) available via a
> configuration option.
>
> I think the way to do this from my patch is to instead retain the
> 'get_priv_for_phy' function 
> within a #ifdef for the configuration option, and select whether to call
> the function or always 
> use the first TSEC instance, again based on the #define.
>
> Given that, as you said, several people, have suggested that the whole
> PHY support in U-Boot 
> needs an overhaul, another question is whether any support for the
> internal PHYs is likely to 
> be implemented before this overhaul.
>
> I guess that if the old functionality is still available within the
> code, then, when the TBI 
> support is implemented then the person that does that has easy access to
> all the code options.
>
> Regards
>
> Michael
>
>   
I like your patch as-is, and would like to apply it. Can you please 
re-send it properly formatted?

I agree that we'll need to identify PHYs by more than just an address. 
In addition to the TBI example that Andy mentioned, MDIO can also be 
bit-banged and I don't see any reason to artificially limit how devices 
are accessed.

Regarding the PHY overhaul, for various non-technical reasons it's going 
slower than I'd hoped. The plan is to make it part of the next release 
cycle, which I guess will close in a couple of months. Hopefully patches 
will dribble in over the next few weeks.

thanks,
Ben