Patchwork [U-Boot,5/8] MXC FEC: Resolve speed before configuring gasket

login
register
mail settings
Submitter Timo Ketola
Date April 12, 2012, 9:33 a.m.
Message ID <1334223234-23383-6-git-send-email-timo@exertus.fi>
Download mbox | patch
Permalink /patch/152014/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Timo Ketola - April 12, 2012, 9:33 a.m.
Signed-off-by: Timo Ketola <timo@exertus.fi>
---
 drivers/net/fec_mxc.c |   41 ++++++++++++++++++++++-------------------
 1 files changed, 22 insertions(+), 19 deletions(-)
Stefano Babic - April 12, 2012, 12:05 p.m.
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
Wolfgang Denk - April 12, 2012, 12:12 p.m.
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
Timo Ketola - April 12, 2012, 1:16 p.m.
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
Stefano Babic - April 12, 2012, 2:31 p.m.
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
Troy Kisky - April 12, 2012, 7:59 p.m.
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;
Timo Ketola - April 12, 2012, 8:12 p.m.
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

Patch

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;