[PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[shh.xie]

From: Shaohui Xie <Shaohui.Xie@freescale.com>

Currently, if phy state is PHY_RUNNING, we always register a CHANGE
when phy works in polling or interrupt ignored, this will make the
adjust_link being called even the phy link did Not changed.

checking the phy link to make sure the link did changed before we
register a CHANGE, if link did not changed, we do nothing.

Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
---
 drivers/net/phy/phy.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 84b1fba..d972851 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -814,6 +814,7 @@ void phy_state_machine(struct work_struct *work)
 	bool needs_aneg = false, do_suspend = false;
 	enum phy_state old_state;
 	int err = 0;
+	int old_link;
 
 	mutex_lock(&phydev->lock);
 
@@ -899,11 +900,18 @@ void phy_state_machine(struct work_struct *work)
 		phydev->adjust_link(phydev->attached_dev);
 		break;
 	case PHY_RUNNING:
-		/* Only register a CHANGE if we are
-		 * polling or ignoring interrupts
+		/* Only register a CHANGE if we are polling or ignoring
+		 * interrupts and link changed since latest checking.
 		 */
-		if (!phy_interrupt_is_valid(phydev))
-			phydev->state = PHY_CHANGELINK;
+		if (!phy_interrupt_is_valid(phydev)) {
+			old_link = phydev->link;
+			err = phy_read_status(phydev);
+			if (err)
+				break;
+
+			if (old_link != phydev->link)
+				phydev->state = PHY_CHANGELINK;
+		}
 		break;
 	case PHY_CHANGELINK:
 		err = phy_read_status(phydev);
-- 
2.1.0.27.g96db324

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Florian Fainelli]

Le 08/13/15 21:23, shh.xie@gmail.com a écrit :
> From: Shaohui Xie <Shaohui.Xie@freescale.com>
> 
> Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> when phy works in polling or interrupt ignored, this will make the
> adjust_link being called even the phy link did Not changed.

Right, which is why most drivers do implement a caching scheme.

> 
> checking the phy link to make sure the link did changed before we
> register a CHANGE, if link did not changed, we do nothing.

With your change we will end-up with virtually polling a PHY twice as
fast as we used to with the RUNNING -> CHANGELINK -> RUNNING transition
(current state transitions), which is probably fine, but puts a bit more
pressure on the (slow) MDIO bus since we end-up with two additional
reads to latch the link status register.

PS: I would appreciate if you could CC me on future libphy submissions.

> 
> Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
> ---
>  drivers/net/phy/phy.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 84b1fba..d972851 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -814,6 +814,7 @@ void phy_state_machine(struct work_struct *work)
>  	bool needs_aneg = false, do_suspend = false;
>  	enum phy_state old_state;
>  	int err = 0;
> +	int old_link;
>  
>  	mutex_lock(&phydev->lock);
>  
> @@ -899,11 +900,18 @@ void phy_state_machine(struct work_struct *work)
>  		phydev->adjust_link(phydev->attached_dev);
>  		break;
>  	case PHY_RUNNING:
> -		/* Only register a CHANGE if we are
> -		 * polling or ignoring interrupts
> +		/* Only register a CHANGE if we are polling or ignoring
> +		 * interrupts and link changed since latest checking.
>  		 */
> -		if (!phy_interrupt_is_valid(phydev))
> -			phydev->state = PHY_CHANGELINK;
> +		if (!phy_interrupt_is_valid(phydev)) {
> +			old_link = phydev->link;
> +			err = phy_read_status(phydev);
> +			if (err)
> +				break;
> +
> +			if (old_link != phydev->link)
> +				phydev->state = PHY_CHANGELINK;
> +		}
>  		break;
>  	case PHY_CHANGELINK:
>  		err = phy_read_status(phydev);
> 


-- 
Florian

RE: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Shaohui Xie]

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Saturday, August 15, 2015 1:02 AM
> To: shh.xie@gmail.com; netdev@vger.kernel.org; davem@davemloft.net
> Cc: Xie Shaohui-B21989
> Subject: Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
> 
> Le 08/13/15 21:23, shh.xie@gmail.com a écrit :
> > From: Shaohui Xie <Shaohui.Xie@freescale.com>
> >
> > Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> > when phy works in polling or interrupt ignored, this will make the
> > adjust_link being called even the phy link did Not changed.
> 
> Right, which is why most drivers do implement a caching scheme.
> 
> >
> > checking the phy link to make sure the link did changed before we
> > register a CHANGE, if link did not changed, we do nothing.
> 
> With your change we will end-up with virtually polling a PHY twice as fast as we
> used to with the RUNNING -> CHANGELINK -> RUNNING transition (current state
> transitions), 
[S.H] Yes. If the link did changed, we'll polling the PHY status twice with my change, 
but if the link did not changed, we'll only need polling the PHY status without 
calling adjust_link, for phy_state_machine works in polling mode at frequency of HZ,
many adjust_link can be saved.

> which is probably fine, but puts a bit more pressure on the (slow)
> MDIO bus since we end-up with two additional reads to latch the link status
> register.
[S.H] Yes, but adjust_link in most driver is more complex than reading PHY status.
And the link won't be changed very frequently.

> 
> PS: I would appreciate if you could CC me on future libphy submissions.
[S.H] OK. 
Thanks for reviewing!

Shaohui

RE: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Shaohui Xie]

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Saturday, August 15, 2015 1:02 AM
> To: shh.xie@gmail.com; netdev@vger.kernel.org; davem@davemloft.net
> Cc: Xie Shaohui-B21989
> Subject: Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
> 
> Le 08/13/15 21:23, shh.xie@gmail.com a écrit :
> > From: Shaohui Xie <Shaohui.Xie@freescale.com>
> >
> > Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> > when phy works in polling or interrupt ignored, this will make the
> > adjust_link being called even the phy link did Not changed.
> 
> Right, which is why most drivers do implement a caching scheme.
> 
> >
> > checking the phy link to make sure the link did changed before we
> > register a CHANGE, if link did not changed, we do nothing.
> 
> With your change we will end-up with virtually polling a PHY twice as fast as we
> used to with the RUNNING -> CHANGELINK -> RUNNING transition (current state
> transitions), which is probably fine, but puts a bit more pressure on the (slow)
> MDIO bus since we end-up with two additional reads to latch the link status
> register.
[S.H] How about put the link checking in state PHY_CHANGELINK, if the link did changed,
Continue to original handle, if the link did not changed, modify the state to PHY_RUNNING?

        case PHY_CHANGELINK:
+               old_link = phydev->link;
                err = phy_read_status(phydev);
                if (err)
                        break;
 
+               if (old_link == phydev->link) {
+                       phydev->state = PHY_RUNNING;
+                       break;
+               }
+

Thanks!
Shaohui

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[David Miller]

From: <shh.xie@gmail.com>
Date: Fri, 14 Aug 2015 12:23:40 +0800

> From: Shaohui Xie <Shaohui.Xie@freescale.com>
> 
> Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> when phy works in polling or interrupt ignored, this will make the
> adjust_link being called even the phy link did Not changed.
> 
> checking the phy link to make sure the link did changed before we
> register a CHANGE, if link did not changed, we do nothing.
> 
> Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>

Applied, thank you.

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Andy Fleming]

On Mon, Aug 17, 2015 at 2:18 PM, David Miller <davem@davemloft.net> wrote:
>
> From: <shh.xie@gmail.com>
> Date: Fri, 14 Aug 2015 12:23:40 +0800
>
> > From: Shaohui Xie <Shaohui.Xie@freescale.com>
> >
> > Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> > when phy works in polling or interrupt ignored, this will make the
> > adjust_link being called even the phy link did Not changed.
> >
> > checking the phy link to make sure the link did changed before we
> > register a CHANGE, if link did not changed, we do nothing.
> >
> > Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
>
> Applied, thank you.

My 3rd attempt to send this, so I apologize. Hopefully this final
attempt is finally in plain text. Clearly, it's been a while...

Florian had some good objections to this patch. Doing a PHY status
read is a very time-consuming operation to do every iteration (it's
probably thousands of cycles), and the idea is supposed to be that the
read only happens in the PHY_CHANGELINK state. Every cycle of the
state machine will read the status, and a change will require that the
status be read twice. Worst of all, if you were to change between two
links quickly, the state machine will NEVER notice the change, because
it is only looking for the link up/down state to change.

One solution might be to modify genphy_read_status() so that it
doesn't do the dummy read up front to clear the sticky low bit. This
*might* make it so that PHY_RUNNING always catches a link change. What
I'm not 100% sure of is whether you can guarantee that any link change
will require the link to first go down. We'd have to read the spec,
though I suspect this would work. The other concern with this solution
is that something might be depending on the old behavior. Certainly
phy_change() would need to make sure to do that "dummy" read sometime
after interrupts are enabled.

Another solution is to revert this patch, and modify adjust_link()
functions to only make changes if the state has changed (They all
should be doing this, anyway, but I can imagine that some
drivers/devices might have difficulty determining what's changed).

Anyone have other ideas? I'm happy to implement the first solution,
but I should note I don't have a lot of test hardware these days.

Andy

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Yegor Yefremov]

On Fri, Aug 14, 2015 at 6:23 AM,  <shh.xie@gmail.com> wrote:
> From: Shaohui Xie <Shaohui.Xie@freescale.com>
>
> Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> when phy works in polling or interrupt ignored, this will make the
> adjust_link being called even the phy link did Not changed.
>
> checking the phy link to make sure the link did changed before we
> register a CHANGE, if link did not changed, we do nothing.
>
> Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
> ---
>  drivers/net/phy/phy.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 84b1fba..d972851 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -814,6 +814,7 @@ void phy_state_machine(struct work_struct *work)
>         bool needs_aneg = false, do_suspend = false;
>         enum phy_state old_state;
>         int err = 0;
> +       int old_link;
>
>         mutex_lock(&phydev->lock);
>
> @@ -899,11 +900,18 @@ void phy_state_machine(struct work_struct *work)
>                 phydev->adjust_link(phydev->attached_dev);
>                 break;
>         case PHY_RUNNING:
> -               /* Only register a CHANGE if we are
> -                * polling or ignoring interrupts
> +               /* Only register a CHANGE if we are polling or ignoring
> +                * interrupts and link changed since latest checking.
>                  */
> -               if (!phy_interrupt_is_valid(phydev))
> -                       phydev->state = PHY_CHANGELINK;
> +               if (!phy_interrupt_is_valid(phydev)) {
> +                       old_link = phydev->link;
> +                       err = phy_read_status(phydev);
> +                       if (err)
> +                               break;
> +
> +                       if (old_link != phydev->link)
> +                               phydev->state = PHY_CHANGELINK;
> +               }
>                 break;
>         case PHY_CHANGELINK:
>                 err = phy_read_status(phydev);

This patch breaks my am335x based board, where one of the CPSW slaves
is connected to IP175D switch chip via RMII interface. Since this
patch packet reception is not working.

Yegor

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Andrew Lunn]

On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote:

> This patch breaks my am335x based board, where one of the CPSW slaves
> is connected to IP175D switch chip via RMII interface. Since this
> patch packet reception is not working.
 
Hi Yegor

Which phy is causing the problem? A PHY inside the switch?

Do you have two back to back PHYs between the MAC and the switch, or
is the CPSW RMII connected directly to the switch?

   Andrew

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Yegor Yefremov]

Hi Andrew,

On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote:
>
>> This patch breaks my am335x based board, where one of the CPSW slaves
>> is connected to IP175D switch chip via RMII interface. Since this
>> patch packet reception is not working.
>
> Hi Yegor
>
> Which phy is causing the problem? A PHY inside the switch?
>
> Do you have two back to back PHYs between the MAC and the switch, or
> is the CPSW RMII connected directly to the switch?

CPSW RMII is connected directly to the switch.

Yegor

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Andrew Lunn]

On Wed, Mar 16, 2016 at 11:23:59PM +0100, Yegor Yefremov wrote:
> Hi Andrew,
> 
> On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote:
> >
> >> This patch breaks my am335x based board, where one of the CPSW slaves
> >> is connected to IP175D switch chip via RMII interface. Since this
> >> patch packet reception is not working.
> >
> > Hi Yegor
> >
> > Which phy is causing the problem? A PHY inside the switch?
> >
> > Do you have two back to back PHYs between the MAC and the switch, or
> > is the CPSW RMII connected directly to the switch?
> 
> CPSW RMII is connected directly to the switch.

So which PHY is causing you problems?

   Andrew

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Yegor Yefremov]

On Thu, Mar 17, 2016 at 12:05 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Mar 16, 2016 at 11:23:59PM +0100, Yegor Yefremov wrote:
>> Hi Andrew,
>>
>> On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote:
>> >
>> >> This patch breaks my am335x based board, where one of the CPSW slaves
>> >> is connected to IP175D switch chip via RMII interface. Since this
>> >> patch packet reception is not working.
>> >
>> > Hi Yegor
>> >
>> > Which phy is causing the problem? A PHY inside the switch?
>> >
>> > Do you have two back to back PHYs between the MAC and the switch, or
>> > is the CPSW RMII connected directly to the switch?
>>
>> CPSW RMII is connected directly to the switch.
>
> So which PHY is causing you problems?

First of all this is the system in question [1]. am335x CPSW has two
slaves and in this particular configuration CPSW is working in Dual
EMAC mode, so that both slaves are independent interfaces eth0 and
eth1.

eth1 is connected to Atheros 8035 PHY via RGMII channel and is working
as expected. eth0 is connected to ICPlus IP175D via RMII interface, so
from CPSW point of view ICPlus IP175D is just an ordinary PHY. Both
Atheros 8035 and ICPlus IP175D are connected via MDIO, so that both of
them will be detected at runtime:

davinci_mdio 4a101000.mdio: davinci mdio revision 1.6
davinci_mdio 4a101000.mdio: detected phy mask f00fff00
Atheros 8035 ethernet 4a101000.mdio:07: GPIO lookup for consumer reset
Atheros 8035 ethernet 4a101000.mdio:07: using lookup tables for GPIO lookup
Atheros 8035 ethernet 4a101000.mdio:07: lookup for GPIO reset failed
libphy: 4a101000.mdio: probed
davinci_mdio 4a101000.mdio: phy[0]: device 4a101000.mdio:00, driver
ICPlus IP175C
davinci_mdio 4a101000.mdio: phy[1]: device 4a101000.mdio:01, driver
ICPlus IP175C
davinci_mdio 4a101000.mdio: phy[2]: device 4a101000.mdio:02, driver
ICPlus IP175C
davinci_mdio 4a101000.mdio: phy[3]: device 4a101000.mdio:03, driver
ICPlus IP175C
davinci_mdio 4a101000.mdio: phy[4]: device 4a101000.mdio:04, driver
ICPlus IP175C
davinci_mdio 4a101000.mdio: phy[5]: device 4a101000.mdio:05, driver unknown
davinci_mdio 4a101000.mdio: phy[6]: device 4a101000.mdio:06, driver unknown
davinci_mdio 4a101000.mdio: phy[7]: device 4a101000.mdio:07, driver
Atheros 8035 ethernet
davinci_mdio 4a101000.mdio: phy[20]: device 4a101000.mdio:14, driver unknown
davinci_mdio 4a101000.mdio: phy[21]: device 4a101000.mdio:15, driver unknown
davinci_mdio 4a101000.mdio: phy[22]: device 4a101000.mdio:16, driver unknown
davinci_mdio 4a101000.mdio: phy[23]: device 4a101000.mdio:17, driver unknown
davinci_mdio 4a101000.mdio: phy[24]: device 4a101000.mdio:18, driver unknown
davinci_mdio 4a101000.mdio: phy[25]: device 4a101000.mdio:19, driver unknown
davinci_mdio 4a101000.mdio: phy[26]: device 4a101000.mdio:1a, driver unknown
davinci_mdio 4a101000.mdio: phy[27]: device 4a101000.mdio:1b, driver unknown

>From ICPlus IP175D point of view eth0 is just a fifth port in the
switch. ICPlus IP175D is in its default configuration, i.e. just acts
as an unmanaged Ethernet switch. As soon as eth0 will be brought up it
has constant 100Mbps connection. I assume, that from MDIO signalling
this connection differs from physical cable insertion.

In prior kernels when I make ip link set eth0 up I get:

cpsw 4a100000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off

After this patch I don't get this state message. Though I cannot see
differences in "ethtool eth0" or "ip addr show eth0" for both kernels.
So I assume, that ICPlus IP175D stays in PHY_RUNNING state.

Let me know, what additional info do you need.

[1] http://www.visionsystems.de/produkte/baltos-ir-5221.html

Yegor

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Andrew Lunn]

On Thu, Mar 17, 2016 at 09:14:17AM +0100, Yegor Yefremov wrote:
> On Thu, Mar 17, 2016 at 12:05 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Wed, Mar 16, 2016 at 11:23:59PM +0100, Yegor Yefremov wrote:
> >> Hi Andrew,
> >>
> >> On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> >> > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote:
> >> >
> >> >> This patch breaks my am335x based board, where one of the CPSW slaves
> >> >> is connected to IP175D switch chip via RMII interface. Since this
> >> >> patch packet reception is not working.
> >> >
> >> > Hi Yegor
> >> >
> >> > Which phy is causing the problem? A PHY inside the switch?
> >> >
> >> > Do you have two back to back PHYs between the MAC and the switch, or
> >> > is the CPSW RMII connected directly to the switch?
> >>
> >> CPSW RMII is connected directly to the switch.
> >
> > So which PHY is causing you problems?

Hi Yegor

Thanks for the details. This helps explain what is going on.

I'm looking at:

http://www.icplus.com.tw/pp-IP175D.html

> First of all this is the system in question [1]. am335x CPSW has two
> slaves and in this particular configuration CPSW is working in Dual
> EMAC mode, so that both slaves are independent interfaces eth0 and
> eth1.
> 
> eth1 is connected to Atheros 8035 PHY via RGMII channel and is working
> as expected. eth0 is connected to ICPlus IP175D via RMII interface, so
> from CPSW point of view ICPlus IP175D is just an ordinary PHY.  Both
> Atheros 8035 and ICPlus IP175D are connected via MDIO, so that both of
> them will be detected at runtime:
> 
> davinci_mdio 4a101000.mdio: davinci mdio revision 1.6
> davinci_mdio 4a101000.mdio: detected phy mask f00fff00
> Atheros 8035 ethernet 4a101000.mdio:07: GPIO lookup for consumer reset
> Atheros 8035 ethernet 4a101000.mdio:07: using lookup tables for GPIO lookup
> Atheros 8035 ethernet 4a101000.mdio:07: lookup for GPIO reset failed
> libphy: 4a101000.mdio: probed
> davinci_mdio 4a101000.mdio: phy[0]: device 4a101000.mdio:00, driver
> ICPlus IP175C
> davinci_mdio 4a101000.mdio: phy[1]: device 4a101000.mdio:01, driver
> ICPlus IP175C
> davinci_mdio 4a101000.mdio: phy[2]: device 4a101000.mdio:02, driver
> ICPlus IP175C
> davinci_mdio 4a101000.mdio: phy[3]: device 4a101000.mdio:03, driver
> ICPlus IP175C
> davinci_mdio 4a101000.mdio: phy[4]: device 4a101000.mdio:04, driver
> ICPlus IP175C

So here we see the 5 PHYs connected to the switch. What we don't see
is what phy it connected to eth0. Since there is no PHY connected to
eth0, you should have a fixed_link node in your device tree.

I assume you are using am335x-baltos-ir5221.dts?

&cpsw_emac0 {
        phy_id = <&davinci_mdio>, <0>;
        phy-mode = "rmii";
        dual_emac_res_vlan = <1>;
};

I'm assuming this means use the PHY at address 0 on the MDIO bus. This
is your problem. You have logically connected PHY0 to the eth0. So if
PHY0 is down, the MAC logically has no carrier. Hence you don't see
any packets. You should be able to quickly prove this. See what
happens when you connect a peer to port0 so the link comes up.

In reality, your hardware does not have a PHY connected to the MAC. It
goes straight to the switch. So you should be using a fixed-link here.

Try something like:

&cpsw_emac0 {
	ixed-link {
        	speed = <100>;
        	full-duplex;
        };

        phy-mode = "rmii";
        dual_emac_res_vlan = <1>;
};

	Andrew

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Yegor Yefremov]

On Thu, Mar 17, 2016 at 2:41 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Mar 17, 2016 at 09:14:17AM +0100, Yegor Yefremov wrote:
>> On Thu, Mar 17, 2016 at 12:05 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > On Wed, Mar 16, 2016 at 11:23:59PM +0100, Yegor Yefremov wrote:
>> >> Hi Andrew,
>> >>
>> >> On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> >> > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote:
>> >> >
>> >> >> This patch breaks my am335x based board, where one of the CPSW slaves
>> >> >> is connected to IP175D switch chip via RMII interface. Since this
>> >> >> patch packet reception is not working.
>> >> >
>> >> > Hi Yegor
>> >> >
>> >> > Which phy is causing the problem? A PHY inside the switch?
>> >> >
>> >> > Do you have two back to back PHYs between the MAC and the switch, or
>> >> > is the CPSW RMII connected directly to the switch?
>> >>
>> >> CPSW RMII is connected directly to the switch.
>> >
>> > So which PHY is causing you problems?
>
> Hi Yegor
>
> Thanks for the details. This helps explain what is going on.
>
> I'm looking at:
>
> http://www.icplus.com.tw/pp-IP175D.html
>
>> First of all this is the system in question [1]. am335x CPSW has two
>> slaves and in this particular configuration CPSW is working in Dual
>> EMAC mode, so that both slaves are independent interfaces eth0 and
>> eth1.
>>
>> eth1 is connected to Atheros 8035 PHY via RGMII channel and is working
>> as expected. eth0 is connected to ICPlus IP175D via RMII interface, so
>> from CPSW point of view ICPlus IP175D is just an ordinary PHY.  Both
>> Atheros 8035 and ICPlus IP175D are connected via MDIO, so that both of
>> them will be detected at runtime:
>>
>> davinci_mdio 4a101000.mdio: davinci mdio revision 1.6
>> davinci_mdio 4a101000.mdio: detected phy mask f00fff00
>> Atheros 8035 ethernet 4a101000.mdio:07: GPIO lookup for consumer reset
>> Atheros 8035 ethernet 4a101000.mdio:07: using lookup tables for GPIO lookup
>> Atheros 8035 ethernet 4a101000.mdio:07: lookup for GPIO reset failed
>> libphy: 4a101000.mdio: probed
>> davinci_mdio 4a101000.mdio: phy[0]: device 4a101000.mdio:00, driver
>> ICPlus IP175C
>> davinci_mdio 4a101000.mdio: phy[1]: device 4a101000.mdio:01, driver
>> ICPlus IP175C
>> davinci_mdio 4a101000.mdio: phy[2]: device 4a101000.mdio:02, driver
>> ICPlus IP175C
>> davinci_mdio 4a101000.mdio: phy[3]: device 4a101000.mdio:03, driver
>> ICPlus IP175C
>> davinci_mdio 4a101000.mdio: phy[4]: device 4a101000.mdio:04, driver
>> ICPlus IP175C
>
> So here we see the 5 PHYs connected to the switch. What we don't see
> is what phy it connected to eth0. Since there is no PHY connected to
> eth0, you should have a fixed_link node in your device tree.
>
> I assume you are using am335x-baltos-ir5221.dts?
>
> &cpsw_emac0 {
>         phy_id = <&davinci_mdio>, <0>;
>         phy-mode = "rmii";
>         dual_emac_res_vlan = <1>;
> };
>
> I'm assuming this means use the PHY at address 0 on the MDIO bus. This
> is your problem. You have logically connected PHY0 to the eth0. So if
> PHY0 is down, the MAC logically has no carrier. Hence you don't see
> any packets. You should be able to quickly prove this. See what
> happens when you connect a peer to port0 so the link comes up.
>
> In reality, your hardware does not have a PHY connected to the MAC. It
> goes straight to the switch. So you should be using a fixed-link here.
>
> Try something like:
>
> &cpsw_emac0 {
>         ixed-link {
>                 speed = <100>;
>                 full-duplex;
>         };
>
>         phy-mode = "rmii";
>         dual_emac_res_vlan = <1>;
> };

After changing cpsw_emac0 entry to:

&cpsw_emac0 {
        phy-mode = "rmii";
        dual_emac_res_vlan = <1>;
        fixed-link {
                speed = <100>;
                full-duplex;
        };
};

I've got packets running in both directions.

Now I have another problem: I cannot disable ICPlus IP175D ports via
SIOCSMIIREG as I could do previously. I get not ioctl errors [1], but
the ports are always on.

[1] https://github.com/visionsystemsgmbh/libonrisc/blob/master/src/onrisc.c#L83

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Andrew Lunn]

> After changing cpsw_emac0 entry to:
> 
> &cpsw_emac0 {
>         phy-mode = "rmii";
>         dual_emac_res_vlan = <1>;
>         fixed-link {
>                 speed = <100>;
>                 full-duplex;
>         };
> };
> 
> I've got packets running in both directions.

Great.
 
> Now I have another problem: I cannot disable ICPlus IP175D ports via
> SIOCSMIIREG as I could do previously. I get not ioctl errors [1], but
> the ports are always on.
> 
> [1] https://github.com/visionsystemsgmbh/libonrisc/blob/master/src/onrisc.c#L83

The MDIO bus is now logically not connected to eth0. Instead you have
the fixed-link mdio device connected to eth0. So SIOCSMIIREG calls on
eth0 now try to manipulate the fixed link phys.

However, i think you can still access the MDIO bus, just use eth1 in
your ioctl call.

You should however consider writing a DSA driver for the switch.

    Andrew

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Yegor Yefremov]

On Thu, Mar 17, 2016 at 4:12 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> After changing cpsw_emac0 entry to:
>>
>> &cpsw_emac0 {
>>         phy-mode = "rmii";
>>         dual_emac_res_vlan = <1>;
>>         fixed-link {
>>                 speed = <100>;
>>                 full-duplex;
>>         };
>> };
>>
>> I've got packets running in both directions.
>
> Great.
>
>> Now I have another problem: I cannot disable ICPlus IP175D ports via
>> SIOCSMIIREG as I could do previously. I get not ioctl errors [1], but
>> the ports are always on.
>>
>> [1] https://github.com/visionsystemsgmbh/libonrisc/blob/master/src/onrisc.c#L83
>
> The MDIO bus is now logically not connected to eth0. Instead you have
> the fixed-link mdio device connected to eth0. So SIOCSMIIREG calls on
> eth0 now try to manipulate the fixed link phys.
>
> However, i think you can still access the MDIO bus, just use eth1 in
> your ioctl call.

Good trick :-)

> You should however consider writing a DSA driver for the switch.

Do you mean SWITCHDEV or is this more or less the same? From time to
time I'm looking at DSA/switchdev patches in the mailing list, but
there seems to be not so many example in kernel. What are the latest
slides, papers aside from Documentation/networking/switchdev.txt?

Yegor

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Andrew Lunn]

> > You should however consider writing a DSA driver for the switch.
> 
> Do you mean SWITCHDEV or is this more or less the same? From time to
> time I'm looking at DSA/switchdev patches in the mailing list, but
> there seems to be not so many example in kernel. What are the latest
> slides, papers aside from Documentation/networking/switchdev.txt?

I don't have access to the datasheet for this device. So i've no idea
how easy/hard it would be.

Documentation/networking/switchdev.txt and
Documentation/networking/dsa/dsa.txt would be a good place to start.

The mv88e6060.c is the simplest driver and gives you the minimum you
need to start with. Looking at the marketing brief, it looks like the
device can do more. But it is best to start simple, get the minimal
accepted, and then incrementally add more.

	  Andrew

Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

[Yegor Yefremov]

On Thu, Mar 17, 2016 at 4:28 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > You should however consider writing a DSA driver for the switch.
>>
>> Do you mean SWITCHDEV or is this more or less the same? From time to
>> time I'm looking at DSA/switchdev patches in the mailing list, but
>> there seems to be not so many example in kernel. What are the latest
>> slides, papers aside from Documentation/networking/switchdev.txt?
>
> I don't have access to the datasheet for this device. So i've no idea
> how easy/hard it would be.
>
> Documentation/networking/switchdev.txt and
> Documentation/networking/dsa/dsa.txt would be a good place to start.
>
> The mv88e6060.c is the simplest driver and gives you the minimum you
> need to start with. Looking at the marketing brief, it looks like the
> device can do more. But it is best to start simple, get the minimal
> accepted, and then incrementally add more.

Will do. Thanks.

Yegor