| Message ID | 1334825735-27992-4-git-send-email-timo@exertus.fi |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
On 19/04/2012 10:55, Timo Ketola wrote: > Gasket needs a different configuration for 10BaseT than for higher > speeds. > > Signed-off-by: Timo Ketola <timo@exertus.fi> > --- > Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
On 4/19/2012 1:55 AM, Timo Ketola wrote: > Gasket needs a different configuration for 10BaseT than for higher > speeds. > > Signed-off-by: Timo Ketola<timo@exertus.fi> > --- > > Changes in v4: > - Rewrapped commit message > > Changes in v2: > - Dropped patches 2 and 3 so this one changed from 5 to 3 > - Rebased to u-boot-imx next > - Removed the remove of 'miiphy_duplex' call > - Changed 'speed == _100BASET' to 'speed != _10BASET' to not to break > _1000BASET > - Changed configuration option to put gasket into RMII mode from > !CONFIG_MII to CONFIG_RMII. I'm not too sure how this should be > done though. !CONFIG_MII is normally used for this but its original > purpose was to enable MII *management* interface, I think... > > drivers/net/fec_mxc.c | 43 ++++++++++++++++++++++++------------------- > 1 files changed, 24 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 824a199..48a69d4 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -440,6 +440,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); > + miiphy_duplex(edev->name, fec->phy_id); > +#endif > + > #if defined(CONFIG_MX25) || defined(CONFIG_MX53) > udelay(100); > /* > @@ -453,9 +469,14 @@ static int fec_open(struct eth_device *edev) > while (readw(&fec->eth->miigsk_enr)& MIIGSK_ENR_READY) > udelay(2); > > -#if !defined(CONFIG_MII) > - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ > - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); > +#if defined(CONFIG_RMII) While this change seems to make sense, it could break some boards. Please split out to a separate patch, and leave as !defined(CONFIG_MII) for this patch. Thanks Troy
On 19.04.2012 22:27, Troy Kisky wrote: > On 4/19/2012 1:55 AM, Timo Ketola wrote: >> -#if !defined(CONFIG_MII) >> - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ >> - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); >> +#if defined(CONFIG_RMII) > > While this change seems to make sense, it could break some boards. Please explain how. Every board using fec_mxc define CONFIG_MII - they have to: #ifndef CONFIG_MII #error "CONFIG_MII has to be defined!" #endif > Please split out to a separate patch, and leave as !defined(CONFIG_MII) > for this patch. Stefano? -- Timo
On 4/19/2012 1:18 PM, Timo Ketola wrote: > On 19.04.2012 22:27, Troy Kisky wrote: >> On 4/19/2012 1:55 AM, Timo Ketola wrote: >>> -#if !defined(CONFIG_MII) >>> - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ >>> - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); >>> +#if defined(CONFIG_RMII) >> >> While this change seems to make sense, it could break some boards. > > Please explain how. Every board using fec_mxc define CONFIG_MII - they > have to: > > #ifndef CONFIG_MII > #error "CONFIG_MII has to be defined!" > #endif Does every board that has a gasket define CONFIG_RMII? Or are you saying that every board with a gasket is already broken?
On 4/19/2012 2:13 PM, Troy Kisky wrote: > On 4/19/2012 1:18 PM, Timo Ketola wrote: >> On 19.04.2012 22:27, Troy Kisky wrote: >>> On 4/19/2012 1:55 AM, Timo Ketola wrote: >>>> -#if !defined(CONFIG_MII) >>>> - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ >>>> - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); >>>> +#if defined(CONFIG_RMII) >>> >>> While this change seems to make sense, it could break some boards. >> >> Please explain how. Every board using fec_mxc define CONFIG_MII - >> they have to: >> >> #ifndef CONFIG_MII >> #error "CONFIG_MII has to be defined!" >> #endif > Does every board that has a gasket define CONFIG_RMII? > > Or are you saying that every board with a gasket is already broken? That should be "Or are you saying that every board using a reduced pin code is alread broken?" > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On 4/19/2012 1:55 AM, Timo Ketola wrote: > Gasket needs a different configuration for 10BaseT than for higher > speeds. > > Signed-off-by: Timo Ketola<timo@exertus.fi> > --- > > Changes in v4: > - Rewrapped commit message > > Changes in v2: > - Dropped patches 2 and 3 so this one changed from 5 to 3 > - Rebased to u-boot-imx next > - Removed the remove of 'miiphy_duplex' call > - Changed 'speed == _100BASET' to 'speed != _10BASET' to not to break > _1000BASET > - Changed configuration option to put gasket into RMII mode from > !CONFIG_MII to CONFIG_RMII. I'm not too sure how this should be > done though. !CONFIG_MII is normally used for this but its original > purpose was to enable MII *management* interface, I think... > > drivers/net/fec_mxc.c | 43 ++++++++++++++++++++++++------------------- > 1 files changed, 24 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 824a199..48a69d4 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -440,6 +440,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); > + miiphy_duplex(edev->name, fec->phy_id); > +#endif > + > #if defined(CONFIG_MX25) || defined(CONFIG_MX53) > udelay(100); > /* > @@ -453,9 +469,14 @@ static int fec_open(struct eth_device *edev) > while (readw(&fec->eth->miigsk_enr)& MIIGSK_ENR_READY) > udelay(2); > > -#if !defined(CONFIG_MII) > - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ > - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); > +#if defined(CONFIG_RMII) > + if (speed != _10BASET) > + /* configure gasket for RMII, 50MHz, no loopback, and no echo */ > + writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); > + else > + /* configure gasket for RMII, 5MHz, no loopback, and no echo */ > + writew(MIIGSK_CFGR_IF_MODE_RMII | MIIGSK_CFGR_FRCONT, > + &fec->eth->miigsk_cfgr); > #else > /* configure gasket for MII, no loopback, and no echo */ > writew(MIIGSK_CFGR_IF_MODE_MII,&fec->eth->miigsk_cfgr); > @@ -474,22 +495,6 @@ static int fec_open(struct eth_device *edev) > } > #endif > Can you fix 10BASET for non-reduced pin count boards as well? Thanks Troy
[undeleted Stefano from CC-list] On 20.04.2012 00:28, Troy Kisky wrote: > On 4/19/2012 1:55 AM, Timo Ketola wrote: ... >> + if (speed != _10BASET) ... > Can you fix 10BASET for non-reduced pin count boards as well? Are they broken? How? If they are, I'm afraid I don't have a board to test. -- Timo
[undeleted Stefano from CC-list] On 20.04.2012 00:23, Troy Kisky wrote: > On 4/19/2012 2:13 PM, Troy Kisky wrote: >> On 4/19/2012 1:18 PM, Timo Ketola wrote: >>> On 19.04.2012 22:27, Troy Kisky wrote: >>>> On 4/19/2012 1:55 AM, Timo Ketola wrote: >>>>> -#if !defined(CONFIG_MII) >>>>> - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ >>>>> - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); >>>>> +#if defined(CONFIG_RMII) >>>> >>>> While this change seems to make sense, it could break some boards. >>> >>> Please explain how. Every board using fec_mxc define CONFIG_MII - they have to: >>> >>> #ifndef CONFIG_MII >>> #error "CONFIG_MII has to be defined!" >>> #endif >> Does every board that has a gasket define CONFIG_RMII? Our board will be first. >> Or are you saying that every board with a gasket is already broken? > That should be > "Or are you saying that every board using a reduced pin code is alread broken?" Yes, if there were one. Is there? -- Timo
On 20/04/2012 06:35, Timo Ketola wrote: > [undeleted Stefano from CC-list] > Hi Timo, hi Troy, > On 20.04.2012 00:23, Troy Kisky wrote: >> On 4/19/2012 2:13 PM, Troy Kisky wrote: >>> On 4/19/2012 1:18 PM, Timo Ketola wrote: >>>> On 19.04.2012 22:27, Troy Kisky wrote: >>>>> On 4/19/2012 1:55 AM, Timo Ketola wrote: >>>>>> -#if !defined(CONFIG_MII) >>>>>> - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ >>>>>> - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); >>>>>> +#if defined(CONFIG_RMII) >>>>> >>>>> While this change seems to make sense, it could break some boards. >>>> >>>> Please explain how. Every board using fec_mxc define CONFIG_MII - >>>> they have to: >>>> >>>> #ifndef CONFIG_MII >>>> #error "CONFIG_MII has to be defined!" >>>> #endif >>> Does every board that has a gasket define CONFIG_RMII? as far as I can see, there are some inconsistencies. All boards define CONFIG_MII, but they really need CONFIG_RMII, because only with my last patch I set the gasket for MII. The driver has always set in a fixed way the gasket for RMII, independently if CONFIG_RMII or CONFIG_MII was set, and that is also wrong. I would say that the configuration file of most boards using fec_mxc must be changed. And then fec_mxc.c does not need at all these lines: #ifndef CONFIG_MII #error "CONFIG_MII has to be defined!" #endif Boards are compiled clean without them. Correct me if I am wrong, but it seems the correct way to do is to drop the unneeded check in the above lines and sets CONFIG_RMII for all boards except the only one (ima3-mx53), that needs really MII. Best regards, Stefano Babic
Dear Stefano, Troy, Scott, On 20.04.2012 10:30, Stefano Babic wrote: > On 20/04/2012 06:35, Timo Ketola wrote: >> [undeleted Stefano from CC-list] >> > > Hi Timo, hi Troy, > >> On 20.04.2012 00:23, Troy Kisky wrote: >>> On 4/19/2012 2:13 PM, Troy Kisky wrote: >>>> On 4/19/2012 1:18 PM, Timo Ketola wrote: >>>>> On 19.04.2012 22:27, Troy Kisky wrote: >>>>>> On 4/19/2012 1:55 AM, Timo Ketola wrote: >>>>>>> -#if !defined(CONFIG_MII) >>>>>>> - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ >>>>>>> - writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr); >>>>>>> +#if defined(CONFIG_RMII) >>>>>> >>>>>> While this change seems to make sense, it could break some boards. >>>>> >>>>> Please explain how. Every board using fec_mxc define CONFIG_MII - >>>>> they have to: >>>>> >>>>> #ifndef CONFIG_MII >>>>> #error "CONFIG_MII has to be defined!" >>>>> #endif >>>> Does every board that has a gasket define CONFIG_RMII? > > as far as I can see, there are some inconsistencies. All boards define > CONFIG_MII, but they really need CONFIG_RMII, because only with my last > patch I set the gasket for MII. The driver has always set in a fixed way > the gasket for RMII, independently if CONFIG_RMII or CONFIG_MII was set, > and that is also wrong. Ah, so, to answer Troy, there really is RMII boards (which maybe was obvious to all others than me; I reasoned in wrong direction: because they would be already broken with this code, there could be none) and they were already broken. > I would say that the configuration file of most boards using fec_mxc > must be changed. > > And then fec_mxc.c does not need at all these lines: > #ifndef CONFIG_MII > #error "CONFIG_MII has to be defined!" > #endif Functionally this does nothing of course but I can imagine the reasoning behind that check: If I understand correctly, fec_mxc depends on MII management interface (for example miiphy_wait_aneg). Then, if CONFIG_MII is not defined, there is inconsistency because configuration says "don't use MII" but fec_mxc still uses it. I don't know whether this causes any confusion. > Boards are compiled clean without them. Correct me if I am wrong, but it > seems the correct way to do is to drop the unneeded check in the above > lines and sets CONFIG_RMII for all boards except the only one > (ima3-mx53), that needs really MII. Agreed regarding CONFIG_RMII. With dropping the check I'm OK either way. Furthermore, I might like to propose to change the name of the configuration variable CONFIG_MII to CONFIG_MII_MGM or something like that. That might reduce confusion (at least I have been quite confused). -- Timo
On 20/04/2012 10:54, Timo Ketola wrote: >> as far as I can see, there are some inconsistencies. All boards define >> CONFIG_MII, but they really need CONFIG_RMII, because only with my last >> patch I set the gasket for MII. The driver has always set in a fixed way >> the gasket for RMII, independently if CONFIG_RMII or CONFIG_MII was set, >> and that is also wrong. Quite right, you have the second board. The ima3 board I added uses also MII instead of RMII. However, I think that something went wrong, as I understand rereading the code. > Functionally this does nothing of course but I can imagine the reasoning > behind that check: If I understand correctly, fec_mxc depends on MII > management interface (for example miiphy_wait_aneg). Then, if CONFIG_MII > is not defined, there is inconsistency because configuration says "don't > use MII" but fec_mxc still uses it. I don't know whether this causes any > confusion. It creates some confusion... > >> Boards are compiled clean without them. Correct me if I am wrong, but it >> seems the correct way to do is to drop the unneeded check in the above >> lines and sets CONFIG_RMII for all boards except the only one >> (ima3-mx53), that needs really MII. > > Agreed regarding CONFIG_RMII. With dropping the check I'm OK either way. > Furthermore, I might like to propose to change the name of the > configuration variable CONFIG_MII to CONFIG_MII_MGM or something like > that. That might reduce confusion (at least I have been quite confused). Support for MX28 added recently CONFIG_FEC_XCV_TYPE. To augment the confusion, CONFIG_FEC_XCV_TYPE is set to MII100 as default, and this let assume that most boards are running with MII if they do not define it. Really all MX5 boards use RMII, not MII. Not only, by setting the RCR register, there is an attempt to set reserved bits on MX5 SOCs, because MX5 defines only bits 0-5. It seems that writing to reserved bits does not produce effects, but it is quite dangerous and not compliant with SOC manual. So at the end we have multiple configuration switches (CONFIG_MII, CONFIG_RMII, and CONFIG_FEC_XCV_TYPE) to set the same thing, and this is not really good ;-(( I assume that setting CONFIG_FEC_XCV_TYPE as default to MII100 was to avoid to break building not MX28 boards, but as you can see generates other problems. I think it is really better that there is *no* default, and each board sets explicitely its own type. Instead of using CONFIG_MII or CONFIG_RMII, we can make use of CONFIG_FEC_XCV_TYPE, as it was already introduced, but making it consistent for all boards. Support for MII in FEC is in u-boot-imx/next in the last patch, and it is not yet merged. I think I am going to drop that patch from my tree, so that we start again from a clean situation (mainline). Best regards, Stefano Babic
On 23.04.2012 10:55, Stefano Babic wrote: > Instead of using CONFIG_MII or CONFIG_RMII, we can make use of > CONFIG_FEC_XCV_TYPE, as it was already introduced, but making it > consistent for all boards. Second for that. -- Timo
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 824a199..48a69d4 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -440,6 +440,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); + miiphy_duplex(edev->name, fec->phy_id); +#endif + #if defined(CONFIG_MX25) || defined(CONFIG_MX53) udelay(100); /* @@ -453,9 +469,14 @@ static int fec_open(struct eth_device *edev) while (readw(&fec->eth->miigsk_enr) & MIIGSK_ENR_READY) udelay(2); -#if !defined(CONFIG_MII) - /* configure gasket for RMII, 50 MHz, no loopback, and no echo */ - writew(MIIGSK_CFGR_IF_MODE_RMII, &fec->eth->miigsk_cfgr); +#if defined(CONFIG_RMII) + if (speed != _10BASET) + /* configure gasket for RMII, 50MHz, no loopback, and no echo */ + writew(MIIGSK_CFGR_IF_MODE_RMII, &fec->eth->miigsk_cfgr); + else + /* configure gasket for RMII, 5MHz, no loopback, and no echo */ + writew(MIIGSK_CFGR_IF_MODE_RMII | MIIGSK_CFGR_FRCONT, + &fec->eth->miigsk_cfgr); #else /* configure gasket for MII, no loopback, and no echo */ writew(MIIGSK_CFGR_IF_MODE_MII, &fec->eth->miigsk_cfgr); @@ -474,22 +495,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;
Gasket needs a different configuration for 10BaseT than for higher speeds. Signed-off-by: Timo Ketola <timo@exertus.fi> --- Changes in v4: - Rewrapped commit message Changes in v2: - Dropped patches 2 and 3 so this one changed from 5 to 3 - Rebased to u-boot-imx next - Removed the remove of 'miiphy_duplex' call - Changed 'speed == _100BASET' to 'speed != _10BASET' to not to break _1000BASET - Changed configuration option to put gasket into RMII mode from !CONFIG_MII to CONFIG_RMII. I'm not too sure how this should be done though. !CONFIG_MII is normally used for this but its original purpose was to enable MII *management* interface, I think... drivers/net/fec_mxc.c | 43 ++++++++++++++++++++++++------------------- 1 files changed, 24 insertions(+), 19 deletions(-)