From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 1/3 v2] net: introduce phylib
Date: Mon, 10 Sep 2012 15:08:00 +0200 [thread overview]
Message-ID: <20120910130800.GB31207@game.jcrosoft.org> (raw)
In-Reply-To: <20120910071420.GS18243@pengutronix.de>
On 09:14 Mon 10 Sep , Sascha Hauer wrote:
> On Sun, Sep 09, 2012 at 05:44:00PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Adapt phylib from linux
> >
> > switch all the driver to it
> >
> > This will allow to have
> > - phy drivers
> > - to only connect the phy at then opening of the device
> > - if the phy is not ready fail on open
> >
> > Same behaviour as in linux and will allow to share code and simplify porting.
> >
>
> [...]
>
> > +
> > +void mii_unregister(struct mii_device *mdev)
> > +{
> > + unregister_device(&mdev->dev);
> > +}
> > +
> > +static int miidev_init(void)
> > +{
> > + register_driver(&miidev_drv);
> > + return 0;
> > +}
> > +
> > +device_initcall(miidev_init);
> > +
>
> Nit: Blank line at EOF
>
> > @@ -0,0 +1,36 @@
> > +/*
> > + * Copyright (c) 2009 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + *
> > + */
> > +
> > +#include <common.h>
> > +#include <phydev.h>
> > +#include <init.h>
> > +
> > +static struct phy_driver generic_phy = {
> > + .drv.name = "Generic PHY",
> > + .phy_id = PHY_ANY_UID,
> > + .phy_id_mask = PHY_ANY_UID,
> > + .features = 0,
> > +};
> > +
> > +static int generic_phy_register(void)
> > +{
> > + return phy_driver_register(&generic_phy);
> > +}
> > +device_initcall(generic_phy_register);
>
> Maybe this should be an earlier initcall? The network devices are mostly
> at device_initcalls. Does it work when the ethernet device gets probed
> before the phy?
no issue the key point is to have the phyi driver before the probe
and the generic phy driver must be the last to be registered
>
> > +
> > +struct bus_type phy_bustype;
> > +static int genphy_config_init(struct phy_device *phydev);
> > +
> > +struct phy_device *phy_device_create(struct mii_device *bus, int addr, int phy_id)
> > +{
> > + struct phy_device *dev;
> > +
> > + /* We allocate the device, and initialize the
> > + * default values */
> > + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +
> > + if (NULL == dev)
> > + return (struct phy_device*) PTR_ERR((void*)-ENOMEM);
> > +
> > + dev->speed = 0;
> > + dev->duplex = -1;
> > + dev->pause = dev->asym_pause = 0;
> > + dev->link = 1;
> > + dev->autoneg = AUTONEG_ENABLE;
> > +
> > + dev->addr = addr;
> > + dev->phy_id = phy_id;
> > +
> > + dev->bus = bus;
> > + dev->dev.parent = bus->parent;
> > + dev->dev.bus = &phy_bustype;
> > +
> > + strcpy(dev->dev.name, "phy");
> > + dev->dev.id = DEVICE_ID_DYNAMIC;
> > +
> > + return dev;
> > +}
> > +/**
> > + * get_phy_id - reads the specified addr for its ID.
> > + * @bus: the target MII bus
> > + * @addr: PHY address on the MII bus
> > + * @phy_id: where to store the ID retrieved.
> > + *
> > + * Description: Reads the ID registers of the PHY at @addr on the
> > + * @bus, stores it in @phy_id and returns zero on success.
> > + */
> > +int get_phy_id(struct mii_device *bus, int addr, u32 *phy_id)
> > +{
> > + int phy_reg;
> > +
> > + /* Grab the bits from PHYIR1, and put them
> > + * in the upper half */
> > + phy_reg = bus->read(bus, addr, MII_PHYSID1);
> > +
> > + if (phy_reg < 0)
> > + return -EIO;
> > +
> > + *phy_id = (phy_reg & 0xffff) << 16;
> > +
> > + /* Grab the bits from PHYIR2, and put them in the lower half */
> > + phy_reg = bus->read(bus, addr, MII_PHYSID2);
> > +
> > + if (phy_reg < 0)
> > + return -EIO;
> > +
> > + *phy_id |= (phy_reg & 0xffff);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * get_phy_device - reads the specified PHY device and returns its @phy_device struct
> > + * @bus: the target MII bus
> > + * @addr: PHY address on the MII bus
> > + *
> > + * Description: Reads the ID registers of the PHY at @addr on the
> > + * @bus, then allocates and returns the phy_device to represent it.
> > + */
> > +struct phy_device *get_phy_device(struct mii_device *bus, int addr)
> > +{
> > + struct phy_device *dev = NULL;
> > + u32 phy_id = 0;
> > + int r;
> > +
> > + r = get_phy_id(bus, addr, &phy_id);
> > + if (r)
> > + return ERR_PTR(r);
> > +
> > + /* If the phy_id is mostly Fs, there is no device there */
> > + if ((phy_id & 0x1fffffff) == 0x1fffffff)
> > + return ERR_PTR(-EIO);
> > +
> > + dev = phy_device_create(bus, addr, phy_id);
> > +
> > + return dev;
> > +}
> > +
> > +/* Automatically gets and returns the PHY device */
> > +int phy_device_connect(struct mii_device *bus, int addr,
> > + void (*adjust_link) (struct mii_device *miidev))
> > +{
> > + struct phy_driver* drv;
> > + struct phy_device* dev = NULL;
> > + unsigned int i;
> > + int ret = -EINVAL;
> > +
> > + if (!bus->phydev) {
> > + if (addr >= 0) {
> > + dev = get_phy_device(bus, addr);
> > + if (IS_ERR(dev)) {
> > + ret = PTR_ERR(dev);
> > + goto fail;
> > + }
> > + } else {
> > + for (i = 0; i < PHY_MAX_ADDR && !bus->phydev; i++) {
> > + dev = get_phy_device(bus, i);
> > + if (IS_ERR(dev))
> > + continue;
> > +
> > + ret = register_device(&dev->dev);
> > + if (ret)
> > + goto fail;
> > + }
> > +
> > + if (i == 32) {
> > + ret = -EIO;
> > + goto fail;
> > + }
> > + }
> > + }
> > +
> > + dev = bus->phydev;
> > + drv = to_phy_driver(dev->dev.driver);
> > +
> > + drv->config_aneg(dev);
> > +
> > + ret = drv->read_status(dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (dev->link)
> > + printf("%dMbps %s duplex link detected\n", dev->speed,
> > + dev->duplex ? "full" : "half");
> > +
> > + if (adjust_link)
> > + adjust_link(bus);
> > +
> > + return 0;
> > +
> > +fail:
> > + if (!IS_ERR(dev))
> > + kfree(dev);
> > + puts("Unable to find a PHY (unknown ID?)\n");
> > + return ret;
> > +}
> > +
> > +/* Generic PHY support and helper functions */
> > +
> > +/**
> > + * genphy_config_advert - sanitize and advertise auto-negotation parameters
> > + * @phydev: target phy_device struct
> > + *
> > + * Description: Writes MII_ADVERTISE with the appropriate values,
> > + * after sanitizing the values to make sure we only advertise
> > + * what is supported. Returns < 0 on error, 0 if the PHY's advertisement
> > + * hasn't changed, and > 0 if it has changed.
> > + */
> > +int genphy_config_advert(struct phy_device *phydev)
> > +{
> > + u32 advertise;
> > + int oldadv, adv;
> > + int err, changed = 0;
> > +
> > + /* Only allow advertising what
> > + * this PHY supports */
> > + phydev->advertising &= phydev->supported;
> > + advertise = phydev->advertising;
> > +
> > + /* Setup standard advertisement */
> > + oldadv = adv = phy_read(phydev, MII_ADVERTISE);
> > +
> > + if (adv < 0)
> > + return adv;
> > +
> > + adv &= ~(ADVERTISE_ALL | ADVERTISE_100BASE4 | ADVERTISE_PAUSE_CAP |
> > + ADVERTISE_PAUSE_ASYM);
> > + adv |= ethtool_adv_to_mii_adv_t(advertise);
> > +
> > + if (adv != oldadv) {
> > + err = phy_write(phydev, MII_ADVERTISE, adv);
> > +
> > + if (err < 0)
> > + return err;
> > + changed = 1;
> > + }
> > +
> > + /* Configure gigabit if it's supported */
> > + if (phydev->supported & (SUPPORTED_1000baseT_Half |
> > + SUPPORTED_1000baseT_Full)) {
> > + oldadv = adv = phy_read(phydev, MII_CTRL1000);
> > +
> > + if (adv < 0)
> > + return adv;
> > +
> > + adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
> > + adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
> > +
> > + if (adv != oldadv) {
> > + err = phy_write(phydev, MII_CTRL1000, adv);
> > +
> > + if (err < 0)
> > + return err;
> > + changed = 1;
> > + }
> > + }
> > +
> > + return changed;
> > +}
> > +
> > +/**
> > + * genphy_setup_forced - configures/forces speed/duplex from @phydev
> > + * @phydev: target phy_device struct
> > + *
> > + * Description: Configures MII_BMCR to force speed/duplex
> > + * to the values in phydev. Assumes that the values are valid.
> > + * Please see phy_sanitize_settings().
> > + */
> > +int genphy_setup_forced(struct phy_device *phydev)
> > +{
> > + int err;
> > + int ctl = 0;
> > +
> > + phydev->pause = phydev->asym_pause = 0;
> > +
> > + if (SPEED_1000 == phydev->speed)
> > + ctl |= BMCR_SPEED1000;
> > + else if (SPEED_100 == phydev->speed)
> > + ctl |= BMCR_SPEED100;
> > +
> > + if (DUPLEX_FULL == phydev->duplex)
> > + ctl |= BMCR_FULLDPLX;
> > +
> > + err = phy_write(phydev, MII_BMCR, ctl);
> > +
> > + return err;
> > +}
> > +
> > +static int phy_aneg_done(struct phy_device *phydev)
> > +{
> > + uint64_t start = get_time_ns();
> > + int ctl;
> > +
> > + while (!is_timeout(start, PHY_AN_TIMEOUT * SECOND)) {
> > + ctl = phy_read(phydev, MII_BMSR);
> > + if (ctl & BMSR_ANEGCOMPLETE) {
> > + phydev->link = 1;
> > + return 0;
> > + }
> > +
> > + /* Restart auto-negotiation if remote fault */
> > + if (ctl & BMSR_RFAULT) {
> > + puts("PHY remote fault detected\n"
> > + "PHY restarting auto-negotiation\n");
> > + phy_write(phydev, MII_BMCR,
> > + BMCR_ANENABLE | BMCR_ANRESTART);
> > + }
> > + }
> > +
> > + phydev->link = 0;
> > + return -ETIMEDOUT;
> > +}
> > +
> > +/**
> > + * genphy_restart_aneg - Enable and Restart Autonegotiation
> > + * @phydev: target phy_device struct
> > + */
> > +int genphy_restart_aneg(struct phy_device *phydev)
> > +{
> > + int ctl;
> > +
> > + ctl = phy_read(phydev, MII_BMCR);
> > +
> > + if (ctl < 0)
> > + return ctl;
> > +
> > + ctl |= (BMCR_ANENABLE | BMCR_ANRESTART);
> > +
> > + /* Don't isolate the PHY if we're negotiating */
> > + ctl &= ~(BMCR_ISOLATE);
> > +
> > + ctl = phy_write(phydev, MII_BMCR, ctl);
> > +
> > + if (ctl < 0)
> > + return ctl;
> > +
> > + return phy_aneg_done(phydev);
> > +}
> > +
> > +/**
> > + * genphy_config_aneg - restart auto-negotiation or write BMCR
> > + * @phydev: target phy_device struct
> > + *
> > + * Description: If auto-negotiation is enabled, we configure the
> > + * advertising, and then restart auto-negotiation. If it is not
> > + * enabled, then we write the BMCR.
> > + */
> > +int genphy_config_aneg(struct phy_device *phydev)
> > +{
> > + int result;
> > +
> > + if (AUTONEG_ENABLE != phydev->autoneg)
> > + return genphy_setup_forced(phydev);
> > +
> > + result = genphy_config_advert(phydev);
> > +
> > + if (result < 0) /* error */
> > + return result;
> > +
> > + if (result == 0) {
> > + /* Advertisement hasn't changed, but maybe aneg was never on to
> > + * begin with? Or maybe phy was isolated? */
> > + int ctl = phy_read(phydev, MII_BMCR);
> > +
> > + if (ctl < 0)
> > + return ctl;
> > +
> > + if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
> > + result = 1; /* do restart aneg */
> > + }
> > +
> > + /* Only restart aneg if we are advertising something different
> > + * than we were before. */
> > + if (result > 0)
> > + result = genphy_restart_aneg(phydev);
> > +
> > + return result;
> > +}
> > +
> > +/**
> > + * genphy_update_link - update link status in @phydev
> > + * @phydev: target phy_device struct
> > + *
> > + * Description: Update the value in phydev->link to reflect the
> > + * current link value. In order to do this, we need to read
> > + * the status register twice, keeping the second value.
> > + */
> > +int genphy_update_link(struct phy_device *phydev)
> > +{
> > + int status;
> > +
> > + /* Do a fake read */
> > + status = phy_read(phydev, MII_BMSR);
> > +
> > + if (status < 0)
> > + return status;
> > +
> > + /* wait phy status update in the phy */
> > + udelay(1000);
> > +
> > + /* Read link and autonegotiation status */
> > + status = phy_read(phydev, MII_BMSR);
> > +
> > + if (status < 0)
> > + return status;
> > +
> > + if ((status & BMSR_LSTATUS) == 0)
> > + phydev->link = 0;
> > + else
> > + phydev->link = 1;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * genphy_read_status - check the link status and update current link state
> > + * @phydev: target phy_device struct
> > + *
> > + * Description: Check the link, then figure out the current state
> > + * by comparing what we advertise with what the link partner
> > + * advertises. Start by checking the gigabit possibilities,
> > + * then move on to 10/100.
> > + */
> > +int genphy_read_status(struct phy_device *phydev)
> > +{
> > + int adv;
> > + int err;
> > + int lpa;
> > + int lpagb = 0;
> > +
> > + /* Update the link, but return if there
> > + * was an error */
> > + err = genphy_update_link(phydev);
> > + if (err)
> > + return err;
> > +
> > + if (AUTONEG_ENABLE == phydev->autoneg) {
> > + if (phydev->supported & (SUPPORTED_1000baseT_Half
> > + | SUPPORTED_1000baseT_Full)) {
> > + lpagb = phy_read(phydev, MII_STAT1000);
> > +
> > + if (lpagb < 0)
> > + return lpagb;
> > +
> > + adv = phy_read(phydev, MII_CTRL1000);
> > +
> > + if (adv < 0)
> > + return adv;
> > +
> > + lpagb &= adv << 2;
> > + }
> > +
> > + lpa = phy_read(phydev, MII_LPA);
> > +
> > + if (lpa < 0)
> > + return lpa;
> > +
> > + adv = phy_read(phydev, MII_ADVERTISE);
> > +
> > + if (adv < 0)
> > + return adv;
> > +
> > + lpa &= adv;
> > +
> > + phydev->speed = SPEED_10;
> > + phydev->duplex = DUPLEX_HALF;
> > + phydev->pause = phydev->asym_pause = 0;
> > +
> > + if (lpagb & (LPA_1000FULL | LPA_1000HALF)) {
> > + phydev->speed = SPEED_1000;
> > +
> > + if (lpagb & LPA_1000FULL)
> > + phydev->duplex = DUPLEX_FULL;
> > + } else if (lpa & (LPA_100FULL | LPA_100HALF)) {
> > + phydev->speed = SPEED_100;
> > +
> > + if (lpa & LPA_100FULL)
> > + phydev->duplex = DUPLEX_FULL;
> > + } else
> > + if (lpa & LPA_10FULL)
> > + phydev->duplex = DUPLEX_FULL;
> > +
> > + if (phydev->duplex == DUPLEX_FULL) {
> > + phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0;
> > + phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0;
> > + }
> > + } else {
> > + int bmcr = phy_read(phydev, MII_BMCR);
> > + if (bmcr < 0)
> > + return bmcr;
> > +
> > + if (bmcr & BMCR_FULLDPLX)
> > + phydev->duplex = DUPLEX_FULL;
> > + else
> > + phydev->duplex = DUPLEX_HALF;
> > +
> > + if (bmcr & BMCR_SPEED1000)
> > + phydev->speed = SPEED_1000;
> > + else if (bmcr & BMCR_SPEED100)
> > + phydev->speed = SPEED_100;
> > + else
> > + phydev->speed = SPEED_10;
> > +
> > + phydev->pause = phydev->asym_pause = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int genphy_config_init(struct phy_device *phydev)
> > +{
> > + int val;
> > + u32 features;
> > +
> > + /* For now, I'll claim that the generic driver supports
> > + * all possible port types */
> > + features = (SUPPORTED_TP | SUPPORTED_MII
> > + | SUPPORTED_AUI | SUPPORTED_FIBRE |
> > + SUPPORTED_BNC);
> > +
> > + /* Do we support autonegotiation? */
> > + val = phy_read(phydev, MII_BMSR);
> > +
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & BMSR_ANEGCAPABLE)
> > + features |= SUPPORTED_Autoneg;
> > +
> > + if (val & BMSR_100FULL)
> > + features |= SUPPORTED_100baseT_Full;
> > + if (val & BMSR_100HALF)
> > + features |= SUPPORTED_100baseT_Half;
> > + if (val & BMSR_10FULL)
> > + features |= SUPPORTED_10baseT_Full;
> > + if (val & BMSR_10HALF)
> > + features |= SUPPORTED_10baseT_Half;
> > +
> > + if (val & BMSR_ESTATEN) {
> > + val = phy_read(phydev, MII_ESTATUS);
> > +
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & ESTATUS_1000_TFULL)
> > + features |= SUPPORTED_1000baseT_Full;
> > + if (val & ESTATUS_1000_THALF)
> > + features |= SUPPORTED_1000baseT_Half;
> > + }
> > +
> > + phydev->supported = features;
> > + phydev->advertising = features;
> > +
> > + return 0;
> > +}
> > +
> > +static int phy_probe(struct device_d *_dev)
> > +{
> > + struct phy_device *dev = to_phy_device(_dev);
> > + struct phy_driver *drv = to_phy_driver(_dev->driver);
> > + struct mii_device *bus = dev->bus;
> > + char str[16];
> > +
> > + bus->phydev = dev;
> > +
> > + if (bus->flags) {
> > + if (bus->flags & MIIDEV_FORCE_10) {
> > + dev->speed = SPEED_10;
> > + dev->duplex = DUPLEX_FULL;
> > + dev->autoneg = !AUTONEG_ENABLE;
> > + }
> > + }
> > +
> > + /* Start out supporting everything. Eventually,
> > + * a controller will attach, and may modify one
> > + * or both of these values */
> > + dev->supported = drv->features;
> > + dev->advertising = drv->features;
> > +
> > + drv->config_init(dev);
>
> Call _dev->driver->probe instead? A phy driver would have to convert the
> device argument to a phy_device using to_phy_device(), but this would be
> the same as other subsystems do it currently.
the phy probe is the for the driver which I did not add but I could
here the config_init is correct
>
> > +
> > +static void phy_remove(struct device_d *dev)
> > +{
> > +}
> > +
> > +struct bus_type phy_bustype = {
> > + .name = "phy",
> > + .match = phy_match,
> > + .probe = phy_probe,
> > + .remove = phy_remove,
>
> Then you could just remove the .remove callback which has the effect
> that a phy drivers .remove function would be called.
the generic code make the remove mandatory
>
> > +};
> > +
> > +int phy_driver_register(struct phy_driver *phydrv)
> > +{
> > + if (!phydrv)
> > + return -1;
>
> Drop this check. A stack dump contains more information than an error
> code that nobody checks. EPERM would be the wrong error code anyway.
>
> > +#define MII_BMSR 0x01 /* Basic mode status register */
> > +#define MII_PHYSID1 0x02 /* PHYS ID 1 */
> > +#define MII_PHYSID2 0x03 /* PHYS ID 2 */
> > +#define MII_ADVERTISE 0x04 /* Advertisement control reg */
> > +#define MII_LPA 0x05 /* Link partner ability reg */
> > +#define MII_EXPANSION 0x06 /* Expansion register */
> > +#define MII_CTRL1000 0x09 /* 1000BASE-T control */
> > +#define MII_STAT1000 0x0a /* 1000BASE-T status */
> > +#define MII_MMD_CTRL 0x0d /* MMD Access Control Register */
> > +#define MII_MMD_DATA 0x0e /* MMD Access Data Register */
>
> Indention broken here.
Just in the patch in the final code it's ok
>
> Otherwise looks good and I'm willing to give it a try.
>
> Please generate the patch with -M next time.
it was IIRC
Best Regards,
J.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
next prev parent reply other threads:[~2012-09-10 13:10 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-09 15:44 Jean-Christophe PLAGNIOL-VILLARD
2012-09-09 15:44 ` [PATCH 2/3] net: catch error on eth_send Jean-Christophe PLAGNIOL-VILLARD
2012-09-09 15:44 ` [PATCH 3/3] net: move the eth_dev status detection at driver level Jean-Christophe PLAGNIOL-VILLARD
2012-09-10 7:14 ` [PATCH 1/3 v2] net: introduce phylib Sascha Hauer
2012-09-10 13:08 ` Jean-Christophe PLAGNIOL-VILLARD [this message]
2012-09-11 9:09 ` Sascha Hauer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120910130800.GB31207@game.jcrosoft.org \
--to=plagnioj@jcrosoft.com \
--cc=barebox@lists.infradead.org \
--cc=s.hauer@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox