Message ID | 1326456604-25137-4-git-send-email-dirk.behme@de.bosch.com |
---|---|
State | Changes Requested |
Headers | show |
> From: Troy Kisky <troy.kisky@boundarydevices.com> > > Allow boards to change what is advertised before an autoneg > restart happens. > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > CC: Troy Kisky <troy.kisky@boundarydevices.com> > CC: Stefano Babic <sbabic@denx.de> > CC: Marek Vasut <marek.vasut@gmail.com> > --- > drivers/net/fec_mxc.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index b50e01c..6271df5 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device > *dev) miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE, > LPA_100FULL | LPA_100HALF | LPA_10FULL | > LPA_10HALF | PHY_ANLPAR_PSB_802_3); > - miiphy_write(dev->name, fec->phy_id, MII_BMCR, > - BMCR_ANENABLE | BMCR_ANRESTART); > - > if (fec->mii_postcall) > ret = fec->mii_postcall(fec->phy_id); > > + miiphy_write(dev->name, fec->phy_id, MII_BMCR, > + BMCR_ANENABLE | BMCR_ANRESTART); > + > return ret; > } I think this might break boards using the mii_postcall.
On 13/01/2012 13:10, Dirk Behme wrote: > From: Troy Kisky <troy.kisky@boundarydevices.com> > > Allow boards to change what is advertised before an autoneg > restart happens. > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > CC: Troy Kisky <troy.kisky@boundarydevices.com> > CC: Stefano Babic <sbabic@denx.de> > CC: Marek Vasut <marek.vasut@gmail.com> > --- > drivers/net/fec_mxc.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index b50e01c..6271df5 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device *dev) > miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE, > LPA_100FULL | LPA_100HALF | LPA_10FULL | > LPA_10HALF | PHY_ANLPAR_PSB_802_3); > - miiphy_write(dev->name, fec->phy_id, MII_BMCR, > - BMCR_ANENABLE | BMCR_ANRESTART); > - > if (fec->mii_postcall) > ret = fec->mii_postcall(fec->phy_id); > > + miiphy_write(dev->name, fec->phy_id, MII_BMCR, > + BMCR_ANENABLE | BMCR_ANRESTART); > + ...but you change the postcall into a precall... The current implementation is correct. Something is set / written , and after that a specific supplied function (if any) is called, then the name postcall. Changing the order is not correct if the fec->mii_postcall thinks (as it should be) that the MII_BMCR register was already written. But you missed to mention why you need such as change : which is the failure / bug you discovered ? Best regards, Stefano Babic
On 1/13/2012 6:54 AM, Stefano Babic wrote: > On 13/01/2012 13:10, Dirk Behme wrote: >> From: Troy Kisky<troy.kisky@boundarydevices.com> >> >> Allow boards to change what is advertised before an autoneg >> restart happens. >> >> Signed-off-by: Troy Kisky<troy.kisky@boundarydevices.com> >> CC: Troy Kisky<troy.kisky@boundarydevices.com> >> CC: Stefano Babic<sbabic@denx.de> >> CC: Marek Vasut<marek.vasut@gmail.com> >> --- >> drivers/net/fec_mxc.c | 6 +++--- >> 1 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c >> index b50e01c..6271df5 100644 >> --- a/drivers/net/fec_mxc.c >> +++ b/drivers/net/fec_mxc.c >> @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device *dev) >> miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE, >> LPA_100FULL | LPA_100HALF | LPA_10FULL | >> LPA_10HALF | PHY_ANLPAR_PSB_802_3); >> - miiphy_write(dev->name, fec->phy_id, MII_BMCR, >> - BMCR_ANENABLE | BMCR_ANRESTART); >> - >> if (fec->mii_postcall) >> ret = fec->mii_postcall(fec->phy_id); >> >> + miiphy_write(dev->name, fec->phy_id, MII_BMCR, >> + BMCR_ANENABLE | BMCR_ANRESTART); >> + > ...but you change the postcall into a precall... I assumed mii_postcall meant mii_post_init_call not mii_post_aneg_call. A rename of the function may be desired. Perhaps we should let the mii_postcall function do the miiphy_write(dev->name, fec->phy_id, MII_BMCR, BMCR_ANENABLE | BMCR_ANRESTART); as well. > > The current implementation is correct. Something is set / written , and > after that a specific supplied function (if any) is called, then the > name postcall. > > Changing the order is not correct if the fec->mii_postcall thinks (as it > should be) that the MII_BMCR register was already written. > > But you missed to mention why you need such as change : which is the > failure / bug you discovered ? I wanted the sabrelite board to be able to remove 1000BaseT advertisement before aneg starts. I think it works either way, but is clearer this way and doesn't rely on luck. > > Best regards, > Stefano Babic >
On 1/13/2012 5:44 AM, Marek Vasut wrote: >> From: Troy Kisky<troy.kisky@boundarydevices.com> >> >> Allow boards to change what is advertised before an autoneg >> restart happens. >> >> Signed-off-by: Troy Kisky<troy.kisky@boundarydevices.com> >> CC: Troy Kisky<troy.kisky@boundarydevices.com> >> CC: Stefano Babic<sbabic@denx.de> >> CC: Marek Vasut<marek.vasut@gmail.com> >> --- >> drivers/net/fec_mxc.c | 6 +++--- >> 1 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c >> index b50e01c..6271df5 100644 >> --- a/drivers/net/fec_mxc.c >> +++ b/drivers/net/fec_mxc.c >> @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device >> *dev) miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE, >> LPA_100FULL | LPA_100HALF | LPA_10FULL | >> LPA_10HALF | PHY_ANLPAR_PSB_802_3); >> - miiphy_write(dev->name, fec->phy_id, MII_BMCR, >> - BMCR_ANENABLE | BMCR_ANRESTART); >> - >> if (fec->mii_postcall) >> ret = fec->mii_postcall(fec->phy_id); >> >> + miiphy_write(dev->name, fec->phy_id, MII_BMCR, >> + BMCR_ANENABLE | BMCR_ANRESTART); >> + >> return ret; >> } > I think this might break boards using the mii_postcall. > Since only 3 boards use this. Perhaps I should code as if (fec->mii_start_aneg_call) ret = fec->mii_start_aneg_call(fec->phy_id); else miiphy_write(dev->name, fec->phy_id, MII_BMCR, BMCR_ANENABLE | BMCR_ANRESTART); and copy the miiphy_write(dev->name, fec->phy_id, MII_BMCR, BMCR_ANENABLE | BMCR_ANRESTART); into all three callbacks? Troy
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index b50e01c..6271df5 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device *dev) miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE, LPA_100FULL | LPA_100HALF | LPA_10FULL | LPA_10HALF | PHY_ANLPAR_PSB_802_3); - miiphy_write(dev->name, fec->phy_id, MII_BMCR, - BMCR_ANENABLE | BMCR_ANRESTART); - if (fec->mii_postcall) ret = fec->mii_postcall(fec->phy_id); + miiphy_write(dev->name, fec->phy_id, MII_BMCR, + BMCR_ANENABLE | BMCR_ANRESTART); + return ret; }