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

michael.firth at bt.com
2008-01-15T09:59:28Z

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