[PATCH 2/4] phylib: Add generic 10G driver

Shaohui Xie Shaohui.Xie at freescale.com
Wed Nov 13 22:56:41 EST 2013


Hello, Florian,

Thank you for reviewing the patches!
Please see my comments inline.

Best Regards, 
Shaohui Xie


> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli at gmail.com]
> Sent: Wednesday, November 13, 2013 1:54 AM
> To: shh.xie at gmail.com
> Cc: linuxppc-dev; linux-kernel at vger.kernel.org; Bucur Madalin-Cristian-
> B32716; Kanetkar Shruti-B44454; Xie Shaohui-B21989
> Subject: Re: [PATCH 2/4] phylib: Add generic 10G driver
> 
> Hello Shaohui,
> 
> 2013/11/11  <shh.xie at gmail.com>:
> > From: Andy Fleming
> >
> > Very incomplete, but will allow for binding an ethernet controller to
> > it.
> >
> > Also, Add XGMII interface type
> 
> So that should be two separate patches, and
> drivers/of/of_net.c::of_get_phy_mode() must be updated to know about
> XMGII.
> 
> >
> > Signed-off-by: Andy Fleming
> 
> Missing Andy's Signed-off-by tag.
[S.H] Will add in next version, I removed it to make git work since Andy's e-mail address is not valid.

> 
> > Signed-off-by: Shaohui Xie <Shaohui.Xie at freescale.com>
> > ---
> >  drivers/net/phy/phy_device.c | 101
> ++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/phy.h          |   1 +
> >  2 files changed, 101 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/phy_device.c
> > b/drivers/net/phy/phy_device.c index 74630e9..30bf2d5 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/module.h>
> >  #include <linux/mii.h>
> >  #include <linux/ethtool.h>
> > +#include <linux/mdio.h>
> >  #include <linux/phy.h>
> >
> >  #include <asm/io.h>
> > @@ -689,6 +690,13 @@ static int genphy_config_advert(struct phy_device
> *phydev)
> >         return changed;
> >  }
> >
> > +int gen10g_config_advert(struct phy_device *dev) {
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(gen10g_config_advert);
> > +
> > +
> >  /**
> >   * genphy_setup_forced - configures/forces speed/duplex from @phydev
> >   * @phydev: target phy_device struct
> > @@ -742,6 +750,12 @@ int genphy_restart_aneg(struct phy_device
> > *phydev)  }  EXPORT_SYMBOL(genphy_restart_aneg);
> >
> > +int gen10g_restart_aneg(struct phy_device *phydev) {
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(gen10g_restart_aneg);
> > +
> >
> >  /**
> >   * genphy_config_aneg - restart auto-negotiation or write BMCR @@
> > -784,6 +798,13 @@ int genphy_config_aneg(struct phy_device *phydev)  }
> > EXPORT_SYMBOL(genphy_config_aneg);
> >
> > +int gen10g_config_aneg(struct phy_device *phydev) {
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(gen10g_config_aneg);
> > +
> > +
> >  /**
> >   * genphy_update_link - update link status in @phydev
> >   * @phydev: target phy_device struct
> > @@ -913,6 +934,35 @@ int genphy_read_status(struct phy_device *phydev)
> > }  EXPORT_SYMBOL(genphy_read_status);
> >
> > +int gen10g_read_status(struct phy_device *phydev) {
> > +       int devad, reg;
> > +       u32 mmd_mask = phydev->c45_ids.devices_in_package;
> > +
> > +       phydev->link = 1;
> > +
> > +       /* For now just lie and say it's 10G all the time */
> > +       phydev->speed = 10000;
> 
> Can you at least make this a little more proof? Something along:
> 
> if (phydev->supported & (SUPPORTED_10000baseT_Full))
>             phydev->speed = SPEED_10000; else if (phydev->supported &
> (SUPPORTED_1000baseT_Full)
>             phydev->speed = SPEED_1000;
[S.H] some 10G PHY only support 10G speed.

> 
> Although ideally we should be reading the relevant registers to figure
> out what to do.
[S.H] Yes, code below will try to read the mmds to get status.

> 
> > +       phydev->duplex = DUPLEX_FULL;
> > +
> > +       for (devad = 0; mmd_mask; devad++, mmd_mask = mmd_mask >> 1) {
> > +               if (!(mmd_mask & 1))
> > +                       continue;
> > +
> > +               /* Read twice because link state is latched and a
> > +                * read moves the current state into the register
> > +                */
> > +               phy_read_mmd(phydev, devad, MDIO_STAT1);
> > +               reg = phy_read_mmd(phydev, devad, MDIO_STAT1);
> > +               if (reg < 0 || !(reg & MDIO_STAT1_LSTATUS))
> > +                       phydev->link = 0;
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(gen10g_read_status);
> > +
> > +
> >  static int genphy_config_init(struct phy_device *phydev)  {
> >         int val;
> > @@ -959,6 +1009,15 @@ static int genphy_config_init(struct phy_device
> > *phydev)
> >
> >         return 0;
> >  }
> > +
> > +static int gen10g_config_init(struct phy_device *phydev) {
> > +       /* Temporarily just say we support everything */
> > +       phydev->supported = phydev->advertising =
> > +SUPPORTED_10000baseT_Full;
> 
> For consistency you should set SUPPORTED_TP, 1000baseT_Full does not make
> sense for anything but twisted pairs AFAIR.
[S.H] OK.

> 
> > +
> > +       return 0;
> > +}
> > +
> >  int genphy_suspend(struct phy_device *phydev)  {
> >         int value;
> > @@ -974,6 +1033,13 @@ int genphy_suspend(struct phy_device *phydev)  }
> > EXPORT_SYMBOL(genphy_suspend);
> >
> > +int gen10g_suspend(struct phy_device *phydev) {
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(gen10g_suspend);
> > +
> > +
> >  int genphy_resume(struct phy_device *phydev)  {
> >         int value;
> > @@ -989,6 +1055,13 @@ int genphy_resume(struct phy_device *phydev)  }
> > EXPORT_SYMBOL(genphy_resume);
> >
> > +int gen10g_resume(struct phy_device *phydev) {
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(gen10g_resume);
> > +
> > +
> >  /**
> >   * phy_probe - probe and init a PHY device
> >   * @dev: device to probe and init
> > @@ -1129,6 +1202,20 @@ static struct phy_driver genphy_driver = {
> >         .driver         = {.owner= THIS_MODULE, },
> >  };
> >
> > +static struct phy_driver gen10g_driver = {
> > +       .phy_id         = 0xffffffff,
> > +       .phy_id_mask    = 0xffffffff,
> > +       .name           = "Generic 10G PHY",
> > +       .config_init    = gen10g_config_init,
> > +       .features       = 0,
> 
> This should be updated to be PHY_10GBIT_FEATURES where
> PHY_10GBIT_FEATURES is defined to contain at least PHY_GBIT_FEATURES.
[S.H] for a generic 10G PHY, what is the feature should be supported?

> 
> > +       .config_aneg    = gen10g_config_aneg,
> > +       .read_status    = gen10g_read_status,
> > +       .suspend        = gen10g_suspend,
> > +       .resume         = gen10g_resume,
> > +       .driver         = {.owner = THIS_MODULE, },
> > +};
> > +
> > +
> >  static int __init phy_init(void)
> >  {
> >         int rc;
> > @@ -1139,13 +1226,25 @@ static int __init phy_init(void)
> >
> >         rc = phy_driver_register(&genphy_driver);
> >         if (rc)
> > -               mdio_bus_exit();
> > +               goto genphy_register_failed;
> > +
> > +       rc = phy_driver_register(&gen10g_driver);
> > +       if (rc)
> > +               goto gen10g_register_failed;
> > +
> > +       return rc;
> > +
> > +gen10g_register_failed:
> > +       phy_driver_unregister(&genphy_driver);
> > +genphy_register_failed:
> > +       mdio_bus_exit();
> 
> As a subsequent patch you could use phy_drivers_register()
[S.H] you mean like phy_drivers_register(gen10g_driver, ARRAY_SIZE(gen10g_driver))?


More information about the Linuxppc-dev mailing list