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

Ben Warren
2008-01-11T05:26:45Z

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