Patchwork [U-Boot,V4,3/8] imx: fec: Resolve speed before configuring gasket

login
register
mail settings
Submitter Timo Ketola
Date April 19, 2012, 8:55 a.m.
Message ID <1334825735-27992-4-git-send-email-timo@exertus.fi>
Download mbox | patch
Permalink /patch/153695/
State Superseded
Delegated to: Stefano Babic
Headers show

Comments

Timo Ketola - April 19, 2012, 8:55 a.m.
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(-)
Stefano Babic - April 19, 2012, 4:16 p.m.
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
Troy Kisky - April 19, 2012, 7:27 p.m.
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
Timo Ketola - April 19, 2012, 8:18 p.m.
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
Troy Kisky - April 19, 2012, 9:13 p.m.
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?
Troy Kisky - April 19, 2012, 9:23 p.m.
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
>
Troy Kisky - April 19, 2012, 9:28 p.m.
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
Timo Ketola - April 20, 2012, 4:25 a.m.
[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
Timo Ketola - April 20, 2012, 4:35 a.m.
[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
Stefano Babic - April 20, 2012, 7:30 a.m.
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
Timo Ketola - April 20, 2012, 8:54 a.m.
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
Stefano Babic - April 23, 2012, 7:55 a.m.
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
Timo Ketola - April 23, 2012, 8:17 a.m.
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

Patch

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;