| Message ID | 1327616505-11669-2-git-send-email-troy.kisky@boundarydevices.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky <troy.kisky@boundarydevices.com> wrote: > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > --- > drivers/net/fec_mxc.c | 35 ++++++++++++++++++++++++++++++----- > drivers/net/fec_mxc.h | 1 + > 2 files changed, 31 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 3fffe79..4d7a38d 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -371,6 +371,20 @@ static int fec_set_hwaddr(struct eth_device *dev) > return 0; > } > > +static void fec_eth_phy_config(struct eth_device *dev) > +{ > +#ifdef CONFIG_PHYLIB > + struct fec_priv *fec = (struct fec_priv *)dev->priv; > + struct phy_device *phydev; > + > + phydev = phy_connect(miiphy_get_dev_by_name(dev->name), > + fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII); > + fec->phydev = phydev; > + if (phydev) > + phy_config(phydev); > +#endif > +} > + > /** > * Start the FEC engine > * @param[in] dev Our device to handle > @@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev) > } > #endif > > - miiphy_wait_aneg(edev); > - speed = miiphy_speed(edev->name, fec->phy_id); > - miiphy_duplex(edev->name, fec->phy_id); > + if (fec->phydev) { > + /* Start up the PHY */ > +#ifdef CONFIG_PHYLIB > + phy_startup(fec->phydev); > +#endif The old miiphy code is not truly compatible with the new PHY Lib code. Please implement full support, rather than relying on the compatibility shim. It should be straightforward, just convert all of the miiphy-style calls into the newer mdio/phy calls. Andy
On 1/29/2012 7:04 PM, Andy Fleming wrote: > On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky > <troy.kisky@boundarydevices.com> wrote: >> Signed-off-by: Troy Kisky<troy.kisky@boundarydevices.com> >> --- >> drivers/net/fec_mxc.c | 35 ++++++++++++++++++++++++++++++----- >> drivers/net/fec_mxc.h | 1 + >> 2 files changed, 31 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c >> index 3fffe79..4d7a38d 100644 >> --- a/drivers/net/fec_mxc.c >> +++ b/drivers/net/fec_mxc.c >> @@ -371,6 +371,20 @@ static int fec_set_hwaddr(struct eth_device *dev) >> return 0; >> } >> >> +static void fec_eth_phy_config(struct eth_device *dev) >> +{ >> +#ifdef CONFIG_PHYLIB >> + struct fec_priv *fec = (struct fec_priv *)dev->priv; >> + struct phy_device *phydev; >> + >> + phydev = phy_connect(miiphy_get_dev_by_name(dev->name), >> + fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII); >> + fec->phydev = phydev; >> + if (phydev) >> + phy_config(phydev); >> +#endif >> +} >> + >> /** >> * Start the FEC engine >> * @param[in] dev Our device to handle >> @@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev) >> } >> #endif >> >> - miiphy_wait_aneg(edev); >> - speed = miiphy_speed(edev->name, fec->phy_id); >> - miiphy_duplex(edev->name, fec->phy_id); >> + if (fec->phydev) { >> + /* Start up the PHY */ >> +#ifdef CONFIG_PHYLIB >> + phy_startup(fec->phydev); >> +#endif > > The old miiphy code is not truly compatible with the new PHY Lib code. > Please implement full support, rather than relying on the > compatibility shim. It should be straightforward, just convert all of > the miiphy-style calls into the newer mdio/phy calls. > > Andy > How can I do this without running the risk of breaking existing boards? Surely, there should be an intermediate stage where both are supported until all boards are converted. Then, my "shim" can be safely removed. Or am I entirely missing you point??? Sorry if I am. Please elaborate on how not to break existing boards which I cannot test. Thanks Troy
On Mon, Jan 30, 2012 at 8:13 PM, Troy Kisky <troy.kisky@boundarydevices.com> wrote: > On 1/29/2012 7:04 PM, Andy Fleming wrote: >> >> On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky >> <troy.kisky@boundarydevices.com> wrote: >>> >>> Signed-off-by: Troy Kisky<troy.kisky@boundarydevices.com> >>> --- >>> drivers/net/fec_mxc.c | 35 ++++++++++++++++++++++++++++++----- >>> drivers/net/fec_mxc.h | 1 + >>> 2 files changed, 31 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c >>> index 3fffe79..4d7a38d 100644 >>> --- a/drivers/net/fec_mxc.c >>> +++ b/drivers/net/fec_mxc.c >>> @@ -371,6 +371,20 @@ static int fec_set_hwaddr(struct eth_device *dev) >>> return 0; >>> } >>> >>> +static void fec_eth_phy_config(struct eth_device *dev) >>> +{ >>> +#ifdef CONFIG_PHYLIB >>> + struct fec_priv *fec = (struct fec_priv *)dev->priv; >>> + struct phy_device *phydev; >>> + >>> + phydev = phy_connect(miiphy_get_dev_by_name(dev->name), >>> + fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII); >>> + fec->phydev = phydev; >>> + if (phydev) >>> + phy_config(phydev); >>> +#endif >>> +} >>> + >>> /** >>> * Start the FEC engine >>> * @param[in] dev Our device to handle >>> @@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev) >>> } >>> #endif >>> >>> - miiphy_wait_aneg(edev); >>> - speed = miiphy_speed(edev->name, fec->phy_id); >>> - miiphy_duplex(edev->name, fec->phy_id); >>> + if (fec->phydev) { >>> + /* Start up the PHY */ >>> +#ifdef CONFIG_PHYLIB >>> + phy_startup(fec->phydev); >>> +#endif >> >> >> The old miiphy code is not truly compatible with the new PHY Lib code. >> Please implement full support, rather than relying on the >> compatibility shim. It should be straightforward, just convert all of >> the miiphy-style calls into the newer mdio/phy calls. >> >> Andy >> > How can I do this without running the risk of breaking existing boards? > Surely, there should be an intermediate stage where both are supported > until all boards are converted. Then, my "shim" can be safely removed. > > Or am I entirely missing you point??? > Sorry if I am. Please elaborate on how not to break existing boards which > I cannot test. Well, it wouldn't take *too* much work to fix it for all boards (Looks like none of the boards do board-specific PHY things. All of the board-specific PHY code is contained, evilly, in the ethernet driver). However, I'm not going to suggest that you have to port the driver fully now. But you need to make the two separate. The proper PHY Lib functions may, at some point, assume that there's proper bus infrastructure behind the bus transactions, and that might cause the redirection through the miiphy shim to do weird things. So allow miiphy to exist, but add the proper mdio initialization code in parallel. The config options will pick which type is used. The problem is that the shim was meant to allow all existing code to continue working while not duplicating too much code. It wasn't meant to allow new code to use the old mechanisms (miiphy_read/miiphy_write, etc). The PHY Lib functions ignore how the miiphy code works, and the miiphy code is unaware of how the PHY Lib code works, so there's potential for things to break horribly. In practice, it's probably fine, but I strongly prefer avoiding hybrid solutions (I think this is the third driver that has tried this, which prompted a patch to mark the miiphy code as deprecated). Andy
On 1/31/2012 5:29 PM, Andy Fleming wrote: > On Mon, Jan 30, 2012 at 8:13 PM, Troy Kisky > <troy.kisky@boundarydevices.com> wrote: >> On 1/29/2012 7:04 PM, Andy Fleming wrote: >>> On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky >>> <troy.kisky@boundarydevices.com> wrote: >>>> Signed-off-by: Troy Kisky<troy.kisky@boundarydevices.com> >>>> --- >>>> drivers/net/fec_mxc.c | 35 ++++++++++++++++++++++++++++++----- >>>> drivers/net/fec_mxc.h | 1 + >>>> 2 files changed, 31 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c >>>> index 3fffe79..4d7a38d 100644 >>>> --- a/drivers/net/fec_mxc.c >>>> +++ b/drivers/net/fec_mxc.c >>>> @@ -371,6 +371,20 @@ static int fec_set_hwaddr(struct eth_device *dev) >>>> return 0; >>>> } >>>> >>>> +static void fec_eth_phy_config(struct eth_device *dev) >>>> +{ >>>> +#ifdef CONFIG_PHYLIB >>>> + struct fec_priv *fec = (struct fec_priv *)dev->priv; >>>> + struct phy_device *phydev; >>>> + >>>> + phydev = phy_connect(miiphy_get_dev_by_name(dev->name), >>>> + fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII); >>>> + fec->phydev = phydev; >>>> + if (phydev) >>>> + phy_config(phydev); >>>> +#endif >>>> +} >>>> + >>>> /** >>>> * Start the FEC engine >>>> * @param[in] dev Our device to handle >>>> @@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev) >>>> } >>>> #endif >>>> >>>> - miiphy_wait_aneg(edev); >>>> - speed = miiphy_speed(edev->name, fec->phy_id); >>>> - miiphy_duplex(edev->name, fec->phy_id); >>>> + if (fec->phydev) { >>>> + /* Start up the PHY */ >>>> +#ifdef CONFIG_PHYLIB >>>> + phy_startup(fec->phydev); >>>> +#endif >>> >>> The old miiphy code is not truly compatible with the new PHY Lib code. >>> Please implement full support, rather than relying on the >>> compatibility shim. It should be straightforward, just convert all of >>> the miiphy-style calls into the newer mdio/phy calls. >>> >>> Andy >>> >> How can I do this without running the risk of breaking existing boards? >> Surely, there should be an intermediate stage where both are supported >> until all boards are converted. Then, my "shim" can be safely removed. >> >> Or am I entirely missing you point??? >> Sorry if I am. Please elaborate on how not to break existing boards which >> I cannot test. > Well, it wouldn't take *too* much work to fix it for all boards (Looks > like none of the boards do board-specific PHY things. All of the > board-specific PHY code is contained, evilly, in the ethernet driver). > However, I'm not going to suggest that you have to port the driver > fully now. But you need to make the two separate. The proper PHY Lib > functions may, at some point, assume that there's proper bus > infrastructure behind the bus transactions, and that might cause the > redirection through the miiphy shim to do weird things. So allow > miiphy to exist, but add the proper mdio initialization code in > parallel. The config options will pick which type is used. > > The problem is that the shim was meant to allow all existing code to > continue working while not duplicating too much code. It wasn't meant > to allow new code to use the old mechanisms (miiphy_read/miiphy_write, > etc). The PHY Lib functions ignore how the miiphy code works, and the > miiphy code is unaware of how the PHY Lib code works, so there's > potential for things to break horribly. In practice, it's probably > fine, but I strongly prefer avoiding hybrid solutions (I think this is > the third driver that has tried this, which prompted a patch to mark > the miiphy code as deprecated). > > Andy > I think I followed you that time. Thanks Troy
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 3fffe79..4d7a38d 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -371,6 +371,20 @@ static int fec_set_hwaddr(struct eth_device *dev) return 0; } +static void fec_eth_phy_config(struct eth_device *dev) +{ +#ifdef CONFIG_PHYLIB + struct fec_priv *fec = (struct fec_priv *)dev->priv; + struct phy_device *phydev; + + phydev = phy_connect(miiphy_get_dev_by_name(dev->name), + fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII); + fec->phydev = phydev; + if (phydev) + phy_config(phydev); +#endif +} + /** * Start the FEC engine * @param[in] dev Our device to handle @@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev) } #endif - miiphy_wait_aneg(edev); - speed = miiphy_speed(edev->name, fec->phy_id); - miiphy_duplex(edev->name, fec->phy_id); + if (fec->phydev) { + /* Start up the PHY */ +#ifdef CONFIG_PHYLIB + phy_startup(fec->phydev); +#endif + speed = fec->phydev->speed; + } else { + miiphy_wait_aneg(edev); + speed = miiphy_speed(edev->name, fec->phy_id); + miiphy_duplex(edev->name, fec->phy_id); + } + + #ifdef CONFIG_MX6Q { u32 ecr = readl(&fec->eth->ecntrl) & ~FEC_ECNTRL_SPEED; @@ -556,7 +580,7 @@ static int fec_init(struct eth_device *dev, bd_t* bd) fec_tbd_init(fec); - if (fec->xcv_type != SEVENWIRE) + if (!fec->phydev && fec->xcv_type != SEVENWIRE) miiphy_restart_aneg(dev); fec_open(dev); @@ -842,7 +866,8 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) debug("got MAC address from fuse: %pM\n", ethaddr); memcpy(edev->enetaddr, ethaddr, 6); } - + /* Configure phy */ + fec_eth_phy_config(edev); return ret; err3: diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h index 1d6ab06..36125b5 100644 --- a/drivers/net/fec_mxc.h +++ b/drivers/net/fec_mxc.h @@ -286,6 +286,7 @@ struct fec_priv { void *base_ptr; int dev_id; int phy_id; + struct phy_device *phydev; int (*mii_postcall)(int); };
Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> --- drivers/net/fec_mxc.c | 35 ++++++++++++++++++++++++++++++----- drivers/net/fec_mxc.h | 1 + 2 files changed, 31 insertions(+), 5 deletions(-)