| Message ID | 1334223234-23383-6-git-send-email-timo@exertus.fi |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On 12/04/2012 11:33, Timo Ketola wrote: > Signed-off-by: Timo Ketola <timo@exertus.fi> > --- Hi Timo, > drivers/net/fec_mxc.c | 41 ++++++++++++++++++++++------------------- > 1 files changed, 22 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 1fdd071..5d11df2 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c Please consider to rebase your patch on u-boot-imx, next branch. There are already a couple of patches related to gasket and MII. > @@ -406,6 +406,22 @@ static int fec_open(struct eth_device *edev) > */ > writel(readl(&fec->eth->ecntrl) | FEC_ECNTRL_ETHER_EN, > &fec->eth->ecntrl); > +#ifdef CONFIG_PHYLIB > + if (!fec->phydev) > + fec_eth_phy_config(edev); > + if (fec->phydev) { > + /* Start up the PHY */ > + phy_startup(fec->phydev); > + speed = fec->phydev->speed; > + } else { > + speed = _100BASET; > + } > +#else > + miiphy_wait_aneg(edev); > + speed = miiphy_speed(edev->name, fec->phy_id); > + // FIXME: useless call: miiphy_duplex(edev->name, fec->phy_id); This is dead code. // comments are not allowed, comment should be real comments, not used to disable code. Why are you disabling ? Please explain the reason and, if it is required, provide a separate patch for this. > +#endif > + > #if defined(CONFIG_MX25) || defined(CONFIG_MX53) > udelay(100); > /* > @@ -418,9 +434,12 @@ static int fec_open(struct eth_device *edev) > /* wait for the gasket to be disabled */ > while (readw(&fec->eth->miigsk_enr) & MIIGSK_ENR_READY) > udelay(2); > - > - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ > - writew(MIIGSK_CFGR_IF_MODE_RMII, &fec->eth->miigsk_cfgr); > + if (speed == _100BASET) > + /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ > + writew(MIIGSK_CFGR_IF_MODE_RMII, &fec->eth->miigsk_cfgr); > + else > + /* configure gasket for RMII, 5 MHz, no loopback, and no echo */ > + writew(MIIGSK_CFGR_IF_MODE_RMII | MIIGSK_CFGR_FRCONT, &fec->eth->miigsk_cfgr); Right, this is correct for 10Mhz Ethernet. Best regards, Stefano Babic
Dear "Timo Ketola", In message <1334223234-23383-6-git-send-email-timo@exertus.fi> you wrote: > Signed-off-by: Timo Ketola <timo@exertus.fi> > --- > drivers/net/fec_mxc.c | 41 ++++++++++++++++++++++------------------- > 1 files changed, 22 insertions(+), 19 deletions(-) ... > + // FIXME: useless call: miiphy_duplex(edev->name, fec->phy_id); ERROR: do not use C99 // comments > + /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ WARNING: line over 80 characters > + writew(MIIGSK_CFGR_IF_MODE_RMII, &fec->eth->miigsk_cfgr); > + else > + /* configure gasket for RMII, 5 MHz, no loopback, and no echo */ > + writew(MIIGSK_CFGR_IF_MODE_RMII | MIIGSK_CFGR_FRCONT, &fec->eth->miigsk_cfgr); WARNING: line over 80 characters Best regards, Wolfgang Denk
On 12.04.2012 15:05, Stefano Babic wrote: > On 12/04/2012 11:33, Timo Ketola wrote: >> Signed-off-by: Timo Ketola<timo@exertus.fi> >> --- a/drivers/net/fec_mxc.c >> +++ b/drivers/net/fec_mxc.c > > Please consider to rebase your patch on u-boot-imx, next branch. There > are already a couple of patches related to gasket and MII. u-boot-imx is separate repository, right? So I have to clone that and apply my patches manually, right? >> + // FIXME: useless call: miiphy_duplex(edev->name, fec->phy_id); > > This is dead code. // comments are not allowed, comment should be real > comments, not used to disable code. Why are you disabling ? Please > explain the reason and, if it is required, provide a separate patch for > this. Return value is discarded and I didn't find any side effects. So it seems to be dead call. If agreed, then I'll edit the patch. -- Timo
On 12/04/2012 15:16, Timo Ketola wrote: > On 12.04.2012 15:05, Stefano Babic wrote: >> On 12/04/2012 11:33, Timo Ketola wrote: >>> Signed-off-by: Timo Ketola<timo@exertus.fi> > >>> --- a/drivers/net/fec_mxc.c >>> +++ b/drivers/net/fec_mxc.c >> >> Please consider to rebase your patch on u-boot-imx, next branch. There >> are already a couple of patches related to gasket and MII. > > u-boot-imx is separate repository, right? Right. > So I have to clone that and > apply my patches manually, right? Yes, and maybe you should rebase some of them. Because we are very near to the release, I put new patches into the -next branch. > >>> + // FIXME: useless call: miiphy_duplex(edev->name, fec->phy_id); >> >> This is dead code. // comments are not allowed, comment should be real >> comments, not used to disable code. Why are you disabling ? Please >> explain the reason and, if it is required, provide a separate patch for >> this. > > Return value is discarded and I didn't find any side effects. So it > seems to be dead call. If agreed, then I'll edit the patch. Return value is discharged, but I presume the function is called to print out the status. The function itself printf "PHY duplex" or "PHY AN duplex", that you drop if you remove the call. Best regards, Stefano Babic
On 4/12/2012 2:33 AM, Timo Ketola wrote: > Signed-off-by: Timo Ketola<timo@exertus.fi> > --- > drivers/net/fec_mxc.c | 41 ++++++++++++++++++++++------------------- > 1 files changed, 22 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 1fdd071..5d11df2 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -406,6 +406,22 @@ static int fec_open(struct eth_device *edev) > */ > writel(readl(&fec->eth->ecntrl) | FEC_ECNTRL_ETHER_EN, > &fec->eth->ecntrl); > +#ifdef CONFIG_PHYLIB > + if (!fec->phydev) > + fec_eth_phy_config(edev); > + if (fec->phydev) { > + /* Start up the PHY */ > + phy_startup(fec->phydev); > + speed = fec->phydev->speed; > + } else { > + speed = _100BASET; > + } > +#else > + miiphy_wait_aneg(edev); > + speed = miiphy_speed(edev->name, fec->phy_id); > + // FIXME: useless call: miiphy_duplex(edev->name, fec->phy_id); > +#endif > + > #if defined(CONFIG_MX25) || defined(CONFIG_MX53) > udelay(100); > /* > @@ -418,9 +434,12 @@ static int fec_open(struct eth_device *edev) > /* wait for the gasket to be disabled */ > while (readw(&fec->eth->miigsk_enr)& MIIGSK_ENR_READY) > udelay(2); > - > - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ > - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); > + if (speed == _100BASET) > + /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ > + writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); > + else > + /* configure gasket for RMII, 5 MHz, no loopback, and no echo */ > + writew(MIIGSK_CFGR_IF_MODE_RMII | MIIGSK_CFGR_FRCONT,&fec->eth->miigsk_cfgr); > This will break gigabit speed. How about if (speed != _10BASET) > /* re-enable the gasket */ > writew(MIIGSK_ENR_EN,&fec->eth->miigsk_enr); > @@ -435,22 +454,6 @@ static int fec_open(struct eth_device *edev) > } > #endif > > -#ifdef CONFIG_PHYLIB > - if (!fec->phydev) > - fec_eth_phy_config(edev); > - if (fec->phydev) { > - /* Start up the PHY */ > - phy_startup(fec->phydev); > - speed = fec->phydev->speed; > - } else { > - speed = _100BASET; > - } > -#else > - miiphy_wait_aneg(edev); > - speed = miiphy_speed(edev->name, fec->phy_id); > - miiphy_duplex(edev->name, fec->phy_id); > -#endif > - > #ifdef FEC_QUIRK_ENET_MAC > { > u32 ecr = readl(&fec->eth->ecntrl)& ~FEC_ECNTRL_SPEED;
On 12.04.2012 22:59, Troy Kisky wrote: > On 4/12/2012 2:33 AM, Timo Ketola wrote: >> Signed-off-by: Timo Ketola<timo@exertus.fi> >> + if (speed == _100BASET) > This will break gigabit speed. How about > > if (speed != _10BASET) Looks fine to me. I'll put it that way in v2. -- Timo
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 1fdd071..5d11df2 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -406,6 +406,22 @@ static int fec_open(struct eth_device *edev) */ writel(readl(&fec->eth->ecntrl) | FEC_ECNTRL_ETHER_EN, &fec->eth->ecntrl); +#ifdef CONFIG_PHYLIB + if (!fec->phydev) + fec_eth_phy_config(edev); + if (fec->phydev) { + /* Start up the PHY */ + phy_startup(fec->phydev); + speed = fec->phydev->speed; + } else { + speed = _100BASET; + } +#else + miiphy_wait_aneg(edev); + speed = miiphy_speed(edev->name, fec->phy_id); + // FIXME: useless call: miiphy_duplex(edev->name, fec->phy_id); +#endif + #if defined(CONFIG_MX25) || defined(CONFIG_MX53) udelay(100); /* @@ -418,9 +434,12 @@ static int fec_open(struct eth_device *edev) /* wait for the gasket to be disabled */ while (readw(&fec->eth->miigsk_enr) & MIIGSK_ENR_READY) udelay(2); - - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ - writew(MIIGSK_CFGR_IF_MODE_RMII, &fec->eth->miigsk_cfgr); + if (speed == _100BASET) + /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ + writew(MIIGSK_CFGR_IF_MODE_RMII, &fec->eth->miigsk_cfgr); + else + /* configure gasket for RMII, 5 MHz, no loopback, and no echo */ + writew(MIIGSK_CFGR_IF_MODE_RMII | MIIGSK_CFGR_FRCONT, &fec->eth->miigsk_cfgr); /* re-enable the gasket */ writew(MIIGSK_ENR_EN, &fec->eth->miigsk_enr); @@ -435,22 +454,6 @@ static int fec_open(struct eth_device *edev) } #endif -#ifdef CONFIG_PHYLIB - if (!fec->phydev) - fec_eth_phy_config(edev); - if (fec->phydev) { - /* Start up the PHY */ - phy_startup(fec->phydev); - speed = fec->phydev->speed; - } else { - speed = _100BASET; - } -#else - miiphy_wait_aneg(edev); - speed = miiphy_speed(edev->name, fec->phy_id); - miiphy_duplex(edev->name, fec->phy_id); -#endif - #ifdef FEC_QUIRK_ENET_MAC { u32 ecr = readl(&fec->eth->ecntrl) & ~FEC_ECNTRL_SPEED;
Signed-off-by: Timo Ketola <timo@exertus.fi> --- drivers/net/fec_mxc.c | 41 ++++++++++++++++++++++------------------- 1 files changed, 22 insertions(+), 19 deletions(-)