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

Ben Warren
2008-01-15T15:36:04Z

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