[PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[Marek Behún]

The mv88e6xxx_port_setup_mac looks if one of the {link, speed, duplex}
parameters is being changed from the current setting, and if not, does
not do anything. This test is wrong in some situations: this method also
has the mode argument, which can also be changed.

For example on Turris Omnia, the mode is PHY_INTERFACE_MODE_RGMII_ID,
which has to be set byt the ->port_set_rgmii_delay method. The test does
not look if mode is being changed (in fact there is currently no method
to determine port mode as phy_interface_t type).

The simplest solution seems to be to drop this test altogether and
simply do the setup when requested.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Vivien Didelot <vivien.didelot@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 2e8b1ab2c6f7..aae63f6515b3 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -420,15 +420,6 @@ int mv88e6xxx_port_setup_mac(struct mv88e6xxx_chip *chip, int port, int link,
 	if (err)
 		return err;
 
-	/* Has anything actually changed? We don't expect the
-	 * interface mode to change without one of the other
-	 * parameters also changing
-	 */
-	if (state.link == link &&
-	    state.speed == speed &&
-	    state.duplex == duplex)
-		return 0;
-
 	/* Port's MAC control must not be changed unless the link is down */
 	err = chip->info->ops->port_set_link(chip, port, 0);
 	if (err)
-- 
2.21.0

[PATCH net-next 2/2] net: fixed_phy: set is_gigabit_capable member when needed

[Marek Behún]

The fixed_phy driver does not set the phydev->is_gigabit_capable member
when the fixed_phy is gigabit capable. This in turn causes
phy_device.c:genphy_read_status function to return unknown phy
parameters (SPEED_UNKNOWN, DUPLEX_UNKNOWN, ...), if the fixed_phy is
created with SPEED_1000.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Vivien Didelot <vivien.didelot@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
---
 drivers/net/phy/fixed_phy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 3ffe46df249e..424b02ad7b7b 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -286,6 +286,7 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
 				 phy->supported);
 		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
 				 phy->supported);
+		phy->is_gigabit_capable = 1;
 		/* fall through */
 	case SPEED_100:
 		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
-- 
2.21.0

Re: [PATCH net-next 2/2] net: fixed_phy: set is_gigabit_capable member when needed

[Andrew Lunn]

On Sun, Aug 11, 2019 at 05:08:12PM +0200, Marek Behún wrote:
> The fixed_phy driver does not set the phydev->is_gigabit_capable member
> when the fixed_phy is gigabit capable.

Neither does any other PHY driver. It should be possible to tell if a
PHY supports 1G by looking at register values. If this does not work
for fixed_link, it means we are missing something in the emulation.
That is what we should be fixing.

Also, this change has nothing to do the lp_advertise, what you
previously said the problem was. At the moment, i don't get the
feeling you have really dug all the way down and really understand the
root causes.

     Andrew

Re: [PATCH net-next 2/2] net: fixed_phy: set is_gigabit_capable member when needed

[Marek Behun]

On Sun, 11 Aug 2019 17:40:01 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Sun, Aug 11, 2019 at 05:08:12PM +0200, Marek Behún wrote:
> > The fixed_phy driver does not set the phydev->is_gigabit_capable member
> > when the fixed_phy is gigabit capable.  
> 
> Neither does any other PHY driver. It should be possible to tell if a
> PHY supports 1G by looking at register values. If this does not work
> for fixed_link, it means we are missing something in the emulation.
> That is what we should be fixing.
> 
> Also, this change has nothing to do the lp_advertise, what you
> previously said the problem was. At the moment, i don't get the
> feeling you have really dug all the way down and really understand the
> root causes.
> 
>      Andrew

Andrew,
is_gigabit_capable is otherwise set only in the phy_probe function.
This function is not called at all for the DSA cpu port fixed_link phy.
Why is that? But I guess it is not important anymore, if CPU and DSA
were converted to phylink in net-next. I shall test it and let you know.
In any case, sorry for the spam.
Marek

Re: [PATCH net-next 2/2] net: fixed_phy: set is_gigabit_capable member when needed

[Heiner Kallweit]

On 11.08.2019 18:08, Marek Behun wrote:
> On Sun, 11 Aug 2019 17:40:01 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>> On Sun, Aug 11, 2019 at 05:08:12PM +0200, Marek Behún wrote:
>>> The fixed_phy driver does not set the phydev->is_gigabit_capable member
>>> when the fixed_phy is gigabit capable.  
>>
>> Neither does any other PHY driver. It should be possible to tell if a
>> PHY supports 1G by looking at register values. If this does not work
>> for fixed_link, it means we are missing something in the emulation.
>> That is what we should be fixing.
>>
>> Also, this change has nothing to do the lp_advertise, what you
>> previously said the problem was. At the moment, i don't get the
>> feeling you have really dug all the way down and really understand the
>> root causes.
>>
>>      Andrew
> 
> Andrew,
> is_gigabit_capable is otherwise set only in the phy_probe function.
> This function is not called at all for the DSA cpu port fixed_link phy.
> Why is that? But I guess it is not important anymore, if CPU and DSA
> were converted to phylink in net-next. I shall test it and let you know.
> In any case, sorry for the spam.
> Marek
> 

A fixed phy should be bound to the genphy driver, but that happens later
in phy_attach_direct(). Maybe the fixed phy device of a CPU port gets
never attached to a net device? This would explain why phy_probe()
doesn't get called.
Following idea: We could swphy let return PHY ID 0xffffffff
(instead of current value 0x00000000). Then the fixed phy device would
be bound to the genphy driver immediately at device registration time.
As a consequence phy_probe() would call genphy_read_abilities() that
sets supported modes properly. This should allow to remove the manual
adding of supported modes in dsa_port_fixed_link_register_of().

One more thing regarding genphy_read_abilities() and fixed phys in general:
swphy sets bit BMSR_ESTATEN, then genphy_read_abilities() reads
MII_ESTATUS to check if and which 1Gbps modes are supported.
MII_ESTATUS however isn't defined in swphy. We're just fortunate
that therefore the default value 0xffff is returned and both
1Gbps modes seem to be supported.

Last but not least I think the calls to genphy_config_init() and
genphy_read_status() in dsa_port_fixed_link_register_of() are
both not needed and could be removed.

Heiner

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[Andrew Lunn]

On Sun, Aug 11, 2019 at 05:08:11PM +0200, Marek Behún wrote:
> The mv88e6xxx_port_setup_mac looks if one of the {link, speed, duplex}
> parameters is being changed from the current setting, and if not, does
> not do anything. This test is wrong in some situations: this method also
> has the mode argument, which can also be changed.
> 
> For example on Turris Omnia, the mode is PHY_INTERFACE_MODE_RGMII_ID,
> which has to be set byt the ->port_set_rgmii_delay method. The test does
> not look if mode is being changed (in fact there is currently no method
> to determine port mode as phy_interface_t type).
> 
> The simplest solution seems to be to drop this test altogether and
> simply do the setup when requested.

Hi Marek

Unfortunately, that code is there for a reason. phylink can call the
->mac_config() method once per second. It is documented that
mac_config() should only reconfigure what, if anything, has changed.
And mv88e6xxx_port_setup_mac() needs to disable the port in order to
change anything. So the change you propose here, under some
conditions, will cause the port to be disabled/enables once per
second.

We need to fix this by expanding the test, not removing it.  My
current _guess_ would be, we need to add a ops->port_get_rgmii_delay()
so we can see if that is what needs configuring.

   Andrew

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[Marek Behun]

On Sun, 11 Aug 2019 17:31:53 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Sun, Aug 11, 2019 at 05:08:11PM +0200, Marek Behún wrote:
> > The mv88e6xxx_port_setup_mac looks if one of the {link, speed, duplex}
> > parameters is being changed from the current setting, and if not, does
> > not do anything. This test is wrong in some situations: this method also
> > has the mode argument, which can also be changed.
> > 
> > For example on Turris Omnia, the mode is PHY_INTERFACE_MODE_RGMII_ID,
> > which has to be set byt the ->port_set_rgmii_delay method. The test does
> > not look if mode is being changed (in fact there is currently no method
> > to determine port mode as phy_interface_t type).
> > 
> > The simplest solution seems to be to drop this test altogether and
> > simply do the setup when requested.  
> 
> Hi Marek
> 
> Unfortunately, that code is there for a reason. phylink can call the
> ->mac_config() method once per second. It is documented that  
> mac_config() should only reconfigure what, if anything, has changed.
> And mv88e6xxx_port_setup_mac() needs to disable the port in order to
> change anything. So the change you propose here, under some
> conditions, will cause the port to be disabled/enables once per
> second.
> 
> We need to fix this by expanding the test, not removing it.  My
> current _guess_ would be, we need to add a ops->port_get_rgmii_delay()
> so we can see if that is what needs configuring.
> 
>    Andrew

Hi Andrew,
what if we added a phy_mode member to struct mv88e6xxx_port, and either
set it to PHY_INTERFACE_MODE_NA in mv88e6xxx_setup, or implemented
methods for converting the switch register values to
PHY_INTERFACE_MODE_* values.
This way we could just add port.mode == mode check to port_setup_mac
method.
I am willing to implement this if you think this could work.

Marek

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[Andrew Lunn]

> what if we added a phy_mode member to struct mv88e6xxx_port, and either
> set it to PHY_INTERFACE_MODE_NA in mv88e6xxx_setup, or implemented
> methods for converting the switch register values to
> PHY_INTERFACE_MODE_* values.

We should read the switch registers. I think you can set the defaults
using strapping pins. And in general, the driver always reads state
from the hardware rather than caching it.

     Andrew

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[Marek Behún]

Hi Andrew,

> We should read the switch registers. I think you can set the defaults
> using strapping pins. And in general, the driver always reads state
> from the hardware rather than caching it.

hmm. The cmode is cached for each port, though. For example
mv88e6390x_port_set_cmode compares the new requested value with the
cached one and doesn't do anything if they are equal.

If mv88e6xxx_port_setup_mac can be called once per second by phylink as
you say, do we really want to read the value via MDIO every time? We
already have cmode cached (read from registers at mv88e6xxx_setup, and
then changed when cmode change is requested). From cmode we can already
differentiate mode in the terms of phy_interface_t, unless it is RGMII,
in which case we would have to read RX/TX timing.

Marek

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[Marek Behún]

On Tue, 13 Aug 2019 17:44:16 +0200
Marek Behún <marek.behun@nic.cz> wrote:

> Hi Andrew,
> 
> > We should read the switch registers. I think you can set the
> > defaults using strapping pins. And in general, the driver always
> > reads state from the hardware rather than caching it.  
> 
> hmm. The cmode is cached for each port, though. For example
> mv88e6390x_port_set_cmode compares the new requested value with the
> cached one and doesn't do anything if they are equal.
> 
> If mv88e6xxx_port_setup_mac can be called once per second by phylink
> as you say, do we really want to read the value via MDIO every time?
> We already have cmode cached (read from registers at mv88e6xxx_setup,
> and then changed when cmode change is requested). From cmode we can
> already differentiate mode in the terms of phy_interface_t, unless it
> is RGMII, in which case we would have to read RX/TX timing.
> 
> Marek

/o\ OK. I see now that mv88e6xxx_port_setup_mac already calls
->port_link_state(), which fills in a struct phylink_link_state, and
already does MDIO communication. Sorry :)
I will try to send a patch which adds the filling of the ->interface
member of the struct phylink_link_state in ->port_link_state() method.

Marek

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[Andrew Lunn]

On Tue, Aug 13, 2019 at 05:44:16PM +0200, Marek Behún wrote:
> Hi Andrew,
> 
> > We should read the switch registers. I think you can set the defaults
> > using strapping pins. And in general, the driver always reads state
> > from the hardware rather than caching it.
> 
> hmm. The cmode is cached for each port, though. For example
> mv88e6390x_port_set_cmode compares the new requested value with the
> cached one and doesn't do anything if they are equal.
> 
> If mv88e6xxx_port_setup_mac can be called once per second by phylink as
> you say, do we really want to read the value via MDIO every time? We
> already have cmode cached (read from registers at mv88e6xxx_setup, and
> then changed when cmode change is requested). From cmode we can already
> differentiate mode in the terms of phy_interface_t, unless it is RGMII,
> in which case we would have to read RX/TX timing.

Hi Marek

cmode gets used a lot, and in interrupt thread context. So i think it
was worth caching it. RGMII Rx/Tx timing is not used much, so i don't
think it is worth caching it. But as you say, using cmode to determine
if the registers actually need to be read does make sense. Most ports
don't use RGMII, they have internal PHYs.

      Andrew

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup

[Sergei Shtylyov]

Hello!

On 11.08.2019 18:08, Marek Behún wrote:

> The mv88e6xxx_port_setup_mac looks if one of the {link, speed, duplex}
> parameters is being changed from the current setting, and if not, does
> not do anything. This test is wrong in some situations: this method also
> has the mode argument, which can also be changed.
> 
> For example on Turris Omnia, the mode is PHY_INTERFACE_MODE_RGMII_ID,
> which has to be set byt the ->port_set_rgmii_delay method. The test does

    s/byt/by/?

> not look if mode is being changed (in fact there is currently no method
> to determine port mode as phy_interface_t type).
> 
> The simplest solution seems to be to drop this test altogether and
> simply do the setup when requested.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Vivien Didelot <vivien.didelot@gmail.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
[...]

MBR, Sergei