[U-Boot-Users] [PATCH 5/5] mpc83xx: UEC: add support for Broadcom BCM5481 Gigabit PHY

[Ben Warren <biggerbadderben@gmail.com>]

Peter Barada wrote:
> On Thu, 2008-01-10 at 15:41 -0500, Ben Warren wrote:
>   
>> Anton Vorontsov wrote:
>>     
>>> This patch adds basic support for Broadcom BCM5481 PHY,
>>> with the quirk needed for at least MPC8360E-RDK.
>>>
>>> Quirk comes from MPC8360E-RDK BSP source, I think author is
>>> Peter Barada <peterb@logicpd.com>, but I'm not sure.
>>>
>>> There are no openly available specifications for that PHY.
>>>
>>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>>> ---
>>>  drivers/qe/uec_phy.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/qe/uec_phy.h |    5 +++
>>>  2 files changed, 82 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/qe/uec_phy.c b/drivers/qe/uec_phy.c
>>> index ca6faa6..6882d03 100644
>>> --- a/drivers/qe/uec_phy.c
>>> +++ b/drivers/qe/uec_phy.c
>>> @@ -237,6 +237,44 @@ static int gbit_config_aneg (struct uec_mii_info *mii_info)
>>>  	return 0;
>>>  }
>>>  
>>> +static int gbit_read_status(struct uec_mii_info *mii_info)
>>> +{
>>> +	u16 status;
>>> +	int err;
>>> +
>>> +	err = genmii_update_link(mii_info);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	if (mii_info->autoneg) {
>>> +		mii_info->pause = 0;
>>> +		status = phy_read(mii_info, MII_1000BASETSTATUS);
>>> +		if (status & (LPA_1000FULL | LPA_1000HALF)) {
>>> +			mii_info->speed = SPEED_1000;
>>> +			if (status & LPA_1000FULL)
>>> +				mii_info->duplex = DUPLEX_FULL;
>>> +			else
>>> +				mii_info->duplex = DUPLEX_HALF;
>>> +		} else {
>>> +			status = phy_read(mii_info, PHY_ANLPAR);
>>> +
>>> +			if (status & (PHY_ANLPAR_10FD | PHY_ANLPAR_TXFD))
>>> +				mii_info->duplex = DUPLEX_FULL;
>>> +			else
>>> +				mii_info->duplex = DUPLEX_HALF;
>>> +			if (status & (PHY_ANLPAR_TXFD | PHY_ANLPAR_TX))
>>> +				mii_info->speed = SPEED_100;
>>> +			else
>>> +				mii_info->speed = SPEED_10;
>>> +		}
>>> +	}
>>> +	/*
>>> +	 * On non-aneg, we assume what we put in BMCR is the speed,
>>> +	 * though magic-aneg shouldn't prevent this case from occurring.
>>> +	 */
>>> +	 return 0;
>>> +}
>>> +
>>>   
>>>       
>> Please roll the 1Gb code into genmii_read_status.  10/100 PHYs should 
>> report 0 in the
>> MII_1000BASETSTATUS register so it should work.
>>     
>>>  static int marvell_config_aneg (struct uec_mii_info *mii_info)
>>>  {
>>>  	/* The Marvell PHY has an errata which requires
>>> @@ -319,6 +357,35 @@ static int genmii_read_status (struct uec_mii_info *mii_info)
>>>  	return 0;
>>>  }
>>>  
>>> +static int bcm_init(struct uec_mii_info *mii_info)
>>> +{
>>> +	gbit_config_aneg(mii_info);
>>> +
>>> +#ifdef CONFIG_MPC8360ERDK
>>> +	{
>>> +		u16 val;
>>> +		int cnt = 50;
>>> +
>>> +		/* Wait for aneg to complete. */
>>> +		do
>>> +			val = phy_read(mii_info, PHY_BMSR);
>>> +		while (--cnt && !(val & PHY_BMSR_AUTN_COMP));
>>> +
>>> +		/* Set RDX clk delay. */
>>> +		phy_write(mii_info, 0x18, 0x7 | (7 << 12));
>>> +
>>> +		val = phy_read(mii_info, 0x18);
>>> +		/* Set RDX-RXC skew. */
>>> +		val |= (1<<8);
>>> +		val |= (7 | (7 << 12));
>>> +		/* Write bits 14:0. */
>>> +		val |= (1<<15);
>>> +		phy_write(mii_info, 0x18, val);
>>> +	}
>>> +#endif
>>>   
>>>       
>> What is it that makes this specific to one board?  Magic numbers are 
>> baaaad, especially for industry-standard memory maps (I'm thinking of 
>> offset 0x18 in case it's not obvious)
>>     
>
> Its specific to the board, in that when hooked up via RGMII the setting
> in the PHY is required to supply adequate delay between the RXD and RXC
> signals instead of using trace lengths to achieve timing.  I know magic
> numbers are baaaaad, but the PHY spec is only available under NDA and as
> such descriptions(including informative constants, etc) can not be
> disclosed.  If it wasn't for this one register setting(after reset), the
> PHY, as used, wouldn't require any PHY-specific code(outside of ID'ng it
> as a 5481).
>
>   
Is this just due to a layout problem on the reference board, or do you 
always need to do this when connecting this MAC/PHY combo via RGMII?  If 
the former, the additional registers needs to be set in board code.  If 
the latter, please do something like this:  Add an 'enet_interface_e 
if_mode' entry to the 'struct uec_mii_info' definition, and fill it in 
when you call 'uec_set_mac_if_mode()' and check the value for RGMII when 
'bcm_init()' is called.

Sorry, but having a *reference* board #ifdef'd in library code is just 
wrong.

Regarding the magic numbers, something like this would be better:

#define MII_BCM_BCM_PRIVATE_0   0x16
#define MII_BCM_BCM_PRIVATE_1   0x17
#define MII_BCM_BCM_PRIVATE_2   0x18

regards,
Ben