Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1695935 > unrolled thread

[PATCH net-next v2 00/10] net: dsa: lan9303: unicast offload, fdb,mdb,STP

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next v2 00/10] net: dsa: lan9303: unicast offload, fdb,mdb,STP Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 18:40 +0200
    [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 18:50 +0200
      Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic  offloading of unicast traffic Andrew Lunn <andrew@lunn.ch> - 2017-07-26 19:30 +0200
        Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic  offloading of unicast traffic Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-27 13:30 +0200
          Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic  offloading of unicast traffic Andrew Lunn <andrew@lunn.ch> - 2017-07-27 15:40 +0200
            Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic  offloading of unicast traffic Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-27 16:10 +0200
    [PATCH net-next v2 03/10] net: dsa: lan9303: Refactor lan9303_enable_packet_processing() Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 18:50 +0200
    [PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link() method Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 18:50 +0200
      Re: [PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link()  method Andrew Lunn <andrew@lunn.ch> - 2017-07-26 19:10 +0200
        Re: [PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link()  method Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-27 12:50 +0200
    [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 18:50 +0200
      Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-07-25 21:20 +0200
        Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-26 14:20 +0200
          Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-07-26 16:40 +0200
            Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-26 17:00 +0200
            Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Andrew Lunn <andrew@lunn.ch> - 2017-07-26 20:00 +0200
              Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO  interface David Miller <davem@davemloft.net> - 2017-07-26 22:10 +0200
                Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-26 22:50 +0200
                  Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Andrew Lunn <andrew@lunn.ch> - 2017-07-26 23:50 +0200
      Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Andrew Lunn <andrew@lunn.ch> - 2017-07-26 19:00 +0200
        Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-28 13:10 +0200
          Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface Andrew Lunn <andrew@lunn.ch> - 2017-07-28 15:40 +0200
    [PATCH net-next v2 09/10] net: dsa: lan9303: Added Documentation/networking/dsa/lan9303.txt Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 19:20 +0200
    [PATCH net-next v2 10/10] net: dsa: lan9303: Only allocate 3 ports Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 19:20 +0200
    [PATCH net-next v2 08/10] net: dsa: lan9303: Added ALR/fdb/mdb handling Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 19:20 +0200
      Re: [PATCH net-next v2 08/10] net: dsa: lan9303: Added ALR/fdb/mdb  handling Andrew Lunn <andrew@lunn.ch> - 2017-07-26 19:50 +0200
        Re: [PATCH net-next v2 08/10] net: dsa: lan9303: Added ALR/fdb/mdb  handling Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-27 13:10 +0200
    [PATCH net-next v2 05/10] net: dsa: added dsa_net_device_to_dsa_port() Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 19:30 +0200
    [PATCH net-next v2 02/10] net: dsa: lan9303: Do not disable/enable switch fabric port 0 at startup Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 20:10 +0200
      Re: [PATCH net-next v2 02/10] net: dsa: lan9303: Do not  disable/enable switch fabric port 0 at startup Andrew Lunn <andrew@lunn.ch> - 2017-07-26 19:00 +0200
        Re: [PATCH net-next v2 02/10] net: dsa: lan9303: Do not  disable/enable switch fabric port 0 at startup Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-27 12:40 +0200
    [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node swe_bcst_throt Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-25 20:20 +0200
      Re: [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node  swe_bcst_throt Andrew Lunn <andrew@lunn.ch> - 2017-07-26 19:20 +0200
        Re: [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node  swe_bcst_throt Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-27 13:00 +0200
          Re: [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node  swe_bcst_throt Andrew Lunn <andrew@lunn.ch> - 2017-07-27 15:30 +0200
            Re: [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node  swe_bcst_throt Jiri Pirko <jiri@resnulli.us> - 2017-07-27 15:40 +0200

Page 1 of 2  [1] 2  Next page →

#1695935 — [PATCH net-next v2 00/10] net: dsa: lan9303: unicast offload, fdb,mdb,STP

This series extends the LAN9303 3 port switch DSA driver. Highlights:
 - Make the MDIO interface work
 - Bridging: Unicast offload
 - Bridging: Added fdb/mdb handling
 - Bridging: STP support
 - Documentation


Changes v1 -> v2:
- sorted out emailing issues, threading and date. And sent from private
  account in order to avoid company disclaimer in emails.
- Removed the three last "work around" patches. But first moved one doc 
  paragraph to the document patch.  

Egil Hjelmeland (10):
  net: dsa: lan9303: Fixed MDIO interface
  net: dsa: lan9303: Do not disable/enable switch fabric port 0 at
    startup
  net: dsa: lan9303: Refactor lan9303_enable_packet_processing()
  net: dsa: lan9303: Added adjust_link() method
  net: dsa: added dsa_net_device_to_dsa_port()
  net: dsa: lan9303: added sysfs node swe_bcst_throt
  net: dsa: lan9303: Added basic offloading of unicast traffic
  net: dsa: lan9303: Added ALR/fdb/mdb handling
  net: dsa: lan9303: Added Documentation/networking/dsa/lan9303.txt
  net: dsa: lan9303: Only allocate 3 ports

 Documentation/networking/dsa/lan9303.txt |  63 +++
 drivers/net/dsa/lan9303-core.c           | 709 ++++++++++++++++++++++++++++---
 drivers/net/dsa/lan9303.h                |  23 +
 drivers/net/dsa/lan9303_i2c.c            |   2 +
 drivers/net/dsa/lan9303_mdio.c           |  34 ++
 include/net/dsa.h                        |   1 +
 net/dsa/slave.c                          |  10 +
 7 files changed, 772 insertions(+), 70 deletions(-)
 create mode 100644 Documentation/networking/dsa/lan9303.txt

-- 
2.11.0

[toc] | [next] | [standalone]

#1695941 — [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

When both user ports are joined to the same bridge, the normal
HW MAC learning is enabled. This means that unicast traffic is forwarded
in HW. Support for STP is also added.

If one of the user ports leave the bridge,
the ports goes back to the initial separated operation.

Added brigde methods port_bridge_join, port_bridge_leave and
port_stp_state_set.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 115 ++++++++++++++++++++++++++++++++++-------
 drivers/net/dsa/lan9303.h      |   1 +
 2 files changed, 98 insertions(+), 18 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index b70acb73aad6..426a75bd89f4 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -18,6 +18,7 @@
 #include <linux/mutex.h>
 #include <linux/mii.h>
 #include <linux/phy.h>
+#include <linux/if_bridge.h>
 
 #include "lan9303.h"
 
@@ -143,6 +144,7 @@
 # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 (0)
 # define LAN9303_SWE_PORT_STATE_LEARNING_PORT0 BIT(1)
 # define LAN9303_SWE_PORT_STATE_BLOCKING_PORT0 BIT(0)
+# define LAN9303_SWE_PORT_STATE_DISABLED_PORT0 (3)
 #define LAN9303_SWE_PORT_MIRROR 0x1846
 # define LAN9303_SWE_PORT_MIRROR_SNIFF_ALL BIT(8)
 # define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT2 BIT(7)
@@ -515,11 +517,30 @@ static int lan9303_enable_packet_processing(struct lan9303 *chip,
 			LAN9303_MAC_TX_CFG_X_TX_ENABLE);
 }
 
+/* forward special tagged packets from port 0 to port 1 *or* port 2 */
+static int lan9303_setup_tagging(struct lan9303 *chip)
+{
+	int ret;
+	/* enable defining the destination port via special VLAN tagging
+	 * for port 0
+	 */
+	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
+				       0x03);
+	if (ret)
+		return ret;
+
+	/* tag incoming packets at port 1 and 2 on their way to port 0 to be
+	 * able to discover their source port
+	 */
+	return lan9303_write_switch_reg(
+		chip, LAN9303_BM_EGRSS_PORT_TYPE,
+		LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
+}
+
 /* We want a special working switch:
  * - do not forward packets between port 1 and 2
  * - forward everything from port 1 to port 0
  * - forward everything from port 2 to port 0
- * - forward special tagged packets from port 0 to port 1 *or* port 2
  */
 static int lan9303_separate_ports(struct lan9303 *chip)
 {
@@ -534,22 +555,6 @@ static int lan9303_separate_ports(struct lan9303 *chip)
 	if (ret)
 		return ret;
 
-	/* enable defining the destination port via special VLAN tagging
-	 * for port 0
-	 */
-	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
-				       0x03);
-	if (ret)
-		return ret;
-
-	/* tag incoming packets at port 1 and 2 on their way to port 0 to be
-	 * able to discover their source port
-	 */
-	ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE,
-			LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
-	if (ret)
-		return ret;
-
 	/* prevent port 1 and 2 from forwarding packets by their own */
 	return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
 				LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 |
@@ -557,6 +562,12 @@ static int lan9303_separate_ports(struct lan9303 *chip)
 				LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
 }
 
+static void lan9303_bridge_ports(struct lan9303 *chip)
+{
+	/* ports bridged: remove mirroring */
+	lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
+}
+
 static int lan9303_handle_reset(struct lan9303 *chip)
 {
 	if (!chip->reset_gpio)
@@ -707,6 +718,10 @@ static int lan9303_setup(struct dsa_switch *ds)
 		return -EINVAL;
 	}
 
+	ret = lan9303_setup_tagging(chip);
+	if (ret)
+		dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
+
 	ret = lan9303_separate_ports(chip);
 	if (ret)
 		dev_err(chip->dev, "failed to separate ports %d\n", ret);
@@ -898,17 +913,81 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	}
 }
 
+static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
+				    struct net_device *br)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
+	if (ds->ports[1].bridge_dev ==  ds->ports[2].bridge_dev) {
+		lan9303_bridge_ports(chip);
+		chip->is_bridged = true;  /* unleash stp_state_set() */
+	}
+
+	return 0;
+}
+
+static void lan9303_port_bridge_leave(struct dsa_switch *ds, int port,
+				      struct net_device *br)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
+	if (chip->is_bridged) {
+		lan9303_separate_ports(chip);
+		chip->is_bridged = false;
+	}
+}
+
+static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
+				       u8 state)
+{
+	int portmask, portstate;
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d, state %d)\n",
+		__func__, port, state);
+	if (!chip->is_bridged)
+		return;
+
+	switch (state) {
+	case BR_STATE_DISABLED:
+		portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
+		break;
+	case BR_STATE_BLOCKING:
+	case BR_STATE_LISTENING:
+		portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
+		break;
+	case BR_STATE_LEARNING:
+		portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
+		break;
+	case BR_STATE_FORWARDING:
+		portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
+		break;
+	default:
+		dev_err(chip->dev, "%s(port %d, state %d)\n",
+			__func__, port, state);
+	}
+	portmask = 0x3 << (port * 2);
+	portstate     <<= (port * 2);
+	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
+				      portstate, portmask);
+}
+
 static struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol = lan9303_get_tag_protocol,
 	.setup = lan9303_setup,
-	.get_strings = lan9303_get_strings,
 	.phy_read = lan9303_phy_read,
 	.phy_write = lan9303_phy_write,
 	.adjust_link = lan9303_adjust_link,
+	.get_strings = lan9303_get_strings,
 	.get_ethtool_stats = lan9303_get_ethtool_stats,
 	.get_sset_count = lan9303_get_sset_count,
 	.port_enable = lan9303_port_enable,
 	.port_disable = lan9303_port_disable,
+	.port_bridge_join       = lan9303_port_bridge_join,
+	.port_bridge_leave      = lan9303_port_bridge_leave,
+	.port_stp_state_set     = lan9303_port_stp_state_set,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index 444d00b460e1..2d74d02c9cef 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -21,6 +21,7 @@ struct lan9303 {
 	struct dsa_switch *ds;
 	struct mutex indirect_mutex; /* protect indexed register access */
 	const struct lan9303_phy_ops *ops;
+	bool is_bridged; /* true if port 1 and 2 is bridged */
 };
 
 extern const struct regmap_access_table lan9303_register_set;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1697425 — Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

Hi Egil

> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
> +static int lan9303_setup_tagging(struct lan9303 *chip)
> +{
> +	int ret;

Blank line please.


> +	/* enable defining the destination port via special VLAN tagging
> +	 * for port 0
> +	 */
> +	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
> +				       0x03);

#define for 0x03.

> +	if (ret)
> +		return ret;
> +
> +	/* tag incoming packets at port 1 and 2 on their way to port 0 to be
> +	 * able to discover their source port
> +	 */
> +	return lan9303_write_switch_reg(
> +		chip, LAN9303_BM_EGRSS_PORT_TYPE,
> +		LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
> +}
> +
>  /* We want a special working switch:
>   * - do not forward packets between port 1 and 2
>   * - forward everything from port 1 to port 0
>   * - forward everything from port 2 to port 0
> - * - forward special tagged packets from port 0 to port 1 *or* port 2
>   */
>  static int lan9303_separate_ports(struct lan9303 *chip)
>  {
> @@ -534,22 +555,6 @@ static int lan9303_separate_ports(struct lan9303 *chip)
>  	if (ret)
>  		return ret;
>  
> -	/* enable defining the destination port via special VLAN tagging
> -	 * for port 0
> -	 */
> -	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
> -				       0x03);
> -	if (ret)
> -		return ret;
> -
> -	/* tag incoming packets at port 1 and 2 on their way to port 0 to be
> -	 * able to discover their source port
> -	 */
> -	ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE,
> -			LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
> -	if (ret)
> -		return ret;
> -
>  	/* prevent port 1 and 2 from forwarding packets by their own */
>  	return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
>  				LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 |
> @@ -557,6 +562,12 @@ static int lan9303_separate_ports(struct lan9303 *chip)
>  				LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
>  }
>  
> +static void lan9303_bridge_ports(struct lan9303 *chip)
> +{
> +	/* ports bridged: remove mirroring */
> +	lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
> +}
> +
>  static int lan9303_handle_reset(struct lan9303 *chip)
>  {
>  	if (!chip->reset_gpio)
> @@ -707,6 +718,10 @@ static int lan9303_setup(struct dsa_switch *ds)
>  		return -EINVAL;
>  	}
>  
> +	ret = lan9303_setup_tagging(chip);
> +	if (ret)
> +		dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
> +
>  	ret = lan9303_separate_ports(chip);
>  	if (ret)
>  		dev_err(chip->dev, "failed to separate ports %d\n", ret);
> @@ -898,17 +913,81 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>  	}
>  }
>  
> +static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
> +				    struct net_device *br)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
> +	if (ds->ports[1].bridge_dev ==  ds->ports[2].bridge_dev) {
> +		lan9303_bridge_ports(chip);
> +		chip->is_bridged = true;  /* unleash stp_state_set() */
> +	}
> +
> +	return 0;
> +}
> +
> +static void lan9303_port_bridge_leave(struct dsa_switch *ds, int port,
> +				      struct net_device *br)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
> +	if (chip->is_bridged) {
> +		lan9303_separate_ports(chip);
> +		chip->is_bridged = false;
> +	}
> +}
> +
> +static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	int portmask, portstate;
> +	struct lan9303 *chip = ds->priv;
> +
> +	dev_dbg(chip->dev, "%s(port %d, state %d)\n",
> +		__func__, port, state);
> +	if (!chip->is_bridged)
> +		return;

I think you are over-simplifying here. Say i have a layer 2 VPN and i
bridge port 1 and the VPN? The software bridge still wants to do STP
on port 1, in order to solve loops.

> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
> +		break;
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
> +		break;
> +	case BR_STATE_LEARNING:
> +		portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
> +		break;
> +	default:
> +		dev_err(chip->dev, "%s(port %d, state %d)\n",
> +			__func__, port, state);
> +	}
> +	portmask = 0x3 << (port * 2);
> +	portstate     <<= (port * 2);
> +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
> +				      portstate, portmask);
> +}




> +
>  static struct dsa_switch_ops lan9303_switch_ops = {
>  	.get_tag_protocol = lan9303_get_tag_protocol,
>  	.setup = lan9303_setup,
> -	.get_strings = lan9303_get_strings,

????

>  	.phy_read = lan9303_phy_read,
>  	.phy_write = lan9303_phy_write,
>  	.adjust_link = lan9303_adjust_link,
> +	.get_strings = lan9303_get_strings,

Please don't include other unrelated changes.

       Andrew

[toc] | [prev] | [next] | [standalone]

#1697912 — Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

On 26. juli 2017 19:24, Andrew Lunn wrote:
> Hi Egil
> 
>> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
>> +static int lan9303_setup_tagging(struct lan9303 *chip)
>> +{
>> +	int ret;
> 
> Blank line please.
> 
> 
>> +	/* enable defining the destination port via special VLAN tagging
>> +	 * for port 0
>> +	 */
>> +	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
>> +				       0x03);
> 
> #define for 0x03.
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* tag incoming packets at port 1 and 2 on their way to port 0 to be
>> +	 * able to discover their source port
>> +	 */
>> +	return lan9303_write_switch_reg(
>> +		chip, LAN9303_BM_EGRSS_PORT_TYPE,
>> +		LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
>> +}
>> +
>>   /* We want a special working switch:
>>    * - do not forward packets between port 1 and 2
>>    * - forward everything from port 1 to port 0
>>    * - forward everything from port 2 to port 0
>> - * - forward special tagged packets from port 0 to port 1 *or* port 2
>>    */
>>   static int lan9303_separate_ports(struct lan9303 *chip)
>>   {
>> @@ -534,22 +555,6 @@ static int lan9303_separate_ports(struct lan9303 *chip)
>>   	if (ret)
>>   		return ret;
>>   
>> -	/* enable defining the destination port via special VLAN tagging
>> -	 * for port 0
>> -	 */
>> -	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
>> -				       0x03);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* tag incoming packets at port 1 and 2 on their way to port 0 to be
>> -	 * able to discover their source port
>> -	 */
>> -	ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE,
>> -			LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
>> -	if (ret)
>> -		return ret;
>> -
>>   	/* prevent port 1 and 2 from forwarding packets by their own */
>>   	return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
>>   				LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 |
>> @@ -557,6 +562,12 @@ static int lan9303_separate_ports(struct lan9303 *chip)
>>   				LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
>>   }
>>   
>> +static void lan9303_bridge_ports(struct lan9303 *chip)
>> +{
>> +	/* ports bridged: remove mirroring */
>> +	lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
>> +}
>> +
>>   static int lan9303_handle_reset(struct lan9303 *chip)
>>   {
>>   	if (!chip->reset_gpio)
>> @@ -707,6 +718,10 @@ static int lan9303_setup(struct dsa_switch *ds)
>>   		return -EINVAL;
>>   	}
>>   
>> +	ret = lan9303_setup_tagging(chip);
>> +	if (ret)
>> +		dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
>> +
>>   	ret = lan9303_separate_ports(chip);
>>   	if (ret)
>>   		dev_err(chip->dev, "failed to separate ports %d\n", ret);
>> @@ -898,17 +913,81 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>>   	}
>>   }
>>   
>> +static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
>> +				    struct net_device *br)
>> +{
>> +	struct lan9303 *chip = ds->priv;
>> +
>> +	dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
>> +	if (ds->ports[1].bridge_dev ==  ds->ports[2].bridge_dev) {
>> +		lan9303_bridge_ports(chip);
>> +		chip->is_bridged = true;  /* unleash stp_state_set() */
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void lan9303_port_bridge_leave(struct dsa_switch *ds, int port,
>> +				      struct net_device *br)
>> +{
>> +	struct lan9303 *chip = ds->priv;
>> +
>> +	dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
>> +	if (chip->is_bridged) {
>> +		lan9303_separate_ports(chip);
>> +		chip->is_bridged = false;
>> +	}
>> +}
>> +
>> +static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
>> +				       u8 state)
>> +{
>> +	int portmask, portstate;
>> +	struct lan9303 *chip = ds->priv;
>> +
>> +	dev_dbg(chip->dev, "%s(port %d, state %d)\n",
>> +		__func__, port, state);
>> +	if (!chip->is_bridged)
>> +		return;
> 
> I think you are over-simplifying here. Say i have a layer 2 VPN and i
> bridge port 1 and the VPN? The software bridge still wants to do STP
> on port 1, in order to solve loops.
> 

Problem is that the mainline lan9303_separate_ports() does its
work by setting port 1 & 2 in STP BLOCKING state (and port 0 in
FORWARDING state). So my understanding is that it would break port
separation if LAN9303_SWE_PORT_STATE is written while the driver
is in the non-bridged state.

I thought the SW bridge would carry doing its STP work even if
there is a port_stp_state_set method on a DSA port?

>> +
>> +	switch (state) {
>> +	case BR_STATE_DISABLED:
>> +		portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
>> +		break;
>> +	case BR_STATE_BLOCKING:
>> +	case BR_STATE_LISTENING:
>> +		portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
>> +		break;
>> +	case BR_STATE_LEARNING:
>> +		portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
>> +		break;
>> +	case BR_STATE_FORWARDING:
>> +		portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
>> +		break;
>> +	default:
>> +		dev_err(chip->dev, "%s(port %d, state %d)\n",
>> +			__func__, port, state);
>> +	}
>> +	portmask = 0x3 << (port * 2);
>> +	portstate     <<= (port * 2);
>> +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
>> +				      portstate, portmask);
>> +}
> 
> 
> 
> 
>> +
>>   static struct dsa_switch_ops lan9303_switch_ops = {
>>   	.get_tag_protocol = lan9303_get_tag_protocol,
>>   	.setup = lan9303_setup,
>> -	.get_strings = lan9303_get_strings,
> 
> ????
> 
>>   	.phy_read = lan9303_phy_read,
>>   	.phy_write = lan9303_phy_write,
>>   	.adjust_link = lan9303_adjust_link,
>> +	.get_strings = lan9303_get_strings,
> 
> Please don't include other unrelated changes.
> 
>         Andrew
> 

[toc] | [prev] | [next] | [standalone]

#1697992 — Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

> >I think you are over-simplifying here. Say i have a layer 2 VPN and i
> >bridge port 1 and the VPN? The software bridge still wants to do STP
> >on port 1, in order to solve loops.
> >
> 
> Problem is that the mainline lan9303_separate_ports() does its
> work by setting port 1 & 2 in STP BLOCKING state (and port 0 in
> FORWARDING state). So my understanding is that it would break port
> separation if LAN9303_SWE_PORT_STATE is written while the driver
> is in the non-bridged state.

If the hardware cannot do it, that is a different matter. But if the
hardware can do STP states per port, you should try to make use of it
here.

> I thought the SW bridge would carry doing its STP work even if
> there is a port_stp_state_set method on a DSA port?

It will, but it means you are dropping frames in software, adding
extra load to the CPU, reducing the available bandwidth for the other
port, etc.

       Andrew

[toc] | [prev] | [next] | [standalone]

#1698010 — Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

On 27. juli 2017 15:31, Andrew Lunn wrote:
>>> I think you are over-simplifying here. Say i have a layer 2 VPN and i
>>> bridge port 1 and the VPN? The software bridge still wants to do STP
>>> on port 1, in order to solve loops.
>>>
>>
>> Problem is that the mainline lan9303_separate_ports() does its
>> work by setting port 1 & 2 in STP BLOCKING state (and port 0 in
>> FORWARDING state). So my understanding is that it would break port
>> separation if LAN9303_SWE_PORT_STATE is written while the driver
>> is in the non-bridged state.
> 
> If the hardware cannot do it, that is a different matter. But if the
> hardware can do STP states per port, you should try to make use of it
> here.
> 

The HW does STP states per port, but not per pair of port.
I can set port 1 in learning, but I can not tell port 2
to ignore addresses learned on port 1. (Except by using VLAN).

Unless somebody can come up with an other way to implement the
port separation, I think this is how it has to be. I suppose
we don't want to break the port separation feature.


>> I thought the SW bridge would carry doing its STP work even if
>> there is a port_stp_state_set method on a DSA port?
> 
> It will, but it means you are dropping frames in software, adding
> extra load to the CPU, reducing the available bandwidth for the other
> port, etc.
> 

That is exactly the case with all traffic with the current mainline
driver.


>         Andrew
> 

Egil

[toc] | [prev] | [next] | [standalone]

#1695942 — [PATCH net-next v2 03/10] net: dsa: lan9303: Refactor lan9303_enable_packet_processing()

lan9303_enable_packet_processing, lan9303_disable_packet_processing()
Pass port number (0,1,2) as parameter instead of port offset.
Simplify accordingly.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 66 ++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c2b53659f58f..0806a0684d55 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -159,9 +159,7 @@
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT1 (BIT(9) | BIT(8))
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 (BIT(1) | BIT(0))
 
-#define LAN9303_PORT_0_OFFSET 0x400
-#define LAN9303_PORT_1_OFFSET 0x800
-#define LAN9303_PORT_2_OFFSET 0xc00
+#define LAN9303_SWITCH_PORT_REG(port, reg0) (0x400 * (port) + (reg0))
 
 /* the built-in PHYs are of type LAN911X */
 #define MII_LAN911X_SPECIAL_MODES 0x12
@@ -457,24 +455,25 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 	return 0;
 }
 
-#define LAN9303_MAC_RX_CFG_OFFS (LAN9303_MAC_RX_CFG_0 - LAN9303_PORT_0_OFFSET)
-#define LAN9303_MAC_TX_CFG_OFFS (LAN9303_MAC_TX_CFG_0 - LAN9303_PORT_0_OFFSET)
-
 static int lan9303_disable_packet_processing(struct lan9303 *chip,
 					     unsigned int port)
 {
 	int ret;
 
 	/* disable RX, but keep register reset default values else */
-	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
-				       LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
+	ret = lan9303_write_switch_reg(
+			chip,
+			LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_RX_CFG_0),
+			LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
 	if (ret)
 		return ret;
 
 	/* disable TX, but keep register reset default values else */
-	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
-				LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
-				LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
+	return lan9303_write_switch_reg(
+			chip,
+			LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_TX_CFG_0),
+			LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
+			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
 }
 
 static int lan9303_enable_packet_processing(struct lan9303 *chip,
@@ -483,17 +482,21 @@ static int lan9303_enable_packet_processing(struct lan9303 *chip,
 	int ret;
 
 	/* enable RX and keep register reset default values else */
-	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
-				       LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
-				       LAN9303_MAC_RX_CFG_X_RX_ENABLE);
+	ret = lan9303_write_switch_reg(
+			chip,
+			LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_RX_CFG_0),
+			LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
+			LAN9303_MAC_RX_CFG_X_RX_ENABLE);
 	if (ret)
 		return ret;
 
 	/* enable TX and keep register reset default values else */
-	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
-				LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
-				LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
-				LAN9303_MAC_TX_CFG_X_TX_ENABLE);
+	return lan9303_write_switch_reg(
+			chip,
+			LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_TX_CFG_0),
+			LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
+			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
+			LAN9303_MAC_TX_CFG_X_TX_ENABLE);
 }
 
 /* We want a special working switch:
@@ -555,12 +558,14 @@ static int lan9303_handle_reset(struct lan9303 *chip)
 /* stop processing packets for all ports */
 static int lan9303_disable_processing(struct lan9303 *chip)
 {
-	int ret;
+	int ret, p;
 
-	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
-	if (ret)
-		return ret;
-	return lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
+	for (p = 1; p <= 2; p++) {
+		ret = lan9303_disable_packet_processing(chip, p);
+		if (ret)
+			return ret;
+	}
+	return 0;
 }
 
 static int lan9303_check_device(struct lan9303 *chip)
@@ -696,7 +701,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
 	unsigned int u, poff;
 	int ret;
 
-	poff = port * 0x400;
+	poff = LAN9303_SWITCH_PORT_REG(port, 0);
 
 	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
 		ret = lan9303_read_switch_reg(chip,
@@ -749,11 +754,8 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 	/* enable internal packet processing */
 	switch (port) {
 	case 1:
-		return lan9303_enable_packet_processing(chip,
-							LAN9303_PORT_1_OFFSET);
 	case 2:
-		return lan9303_enable_packet_processing(chip,
-							LAN9303_PORT_2_OFFSET);
+		return lan9303_enable_packet_processing(chip, port);
 	default:
 		dev_dbg(chip->dev,
 			"Error: request to power up invalid port %d\n", port);
@@ -770,13 +772,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	/* disable internal packet processing */
 	switch (port) {
 	case 1:
-		lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
-		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
-				  MII_BMCR, BMCR_PDOWN);
-		break;
 	case 2:
-		lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
-		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
+		lan9303_disable_packet_processing(chip, port);
+		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
 				  MII_BMCR, BMCR_PDOWN);
 		break;
 	default:
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1695947 — [PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link() method

This makes the driver react to device tree "fixed-link" declaration
on CPU port.

- turn off autonegotiation
- force speed 10 or 100 mb/s
- force duplex mode

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 0806a0684d55..be6d78f45a5f 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -17,6 +17,7 @@
 #include <linux/regmap.h>
 #include <linux/mutex.h>
 #include <linux/mii.h>
+#include <linux/phy.h>
 
 #include "lan9303.h"
 
@@ -746,6 +747,37 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	return chip->ops->phy_write(chip, phy, regnum, val);
 }
 
+static void lan9303_adjust_link(struct dsa_switch *ds, int port,
+				struct phy_device *phydev)
+{
+	struct lan9303 *chip = ds->priv;
+
+	int ctl, res;
+
+	ctl = lan9303_phy_read(ds, port, MII_BMCR);
+
+	if (!phy_is_pseudo_fixed_link(phydev))
+		return;
+
+	ctl &= ~BMCR_ANENABLE;
+	if (phydev->speed == SPEED_100)
+		ctl |= BMCR_SPEED100;
+
+	if (phydev->duplex == DUPLEX_FULL)
+		ctl |= BMCR_FULLDPLX;
+
+	res =  lan9303_phy_write(ds, port, MII_BMCR, ctl);
+
+	if (port == chip->phy_addr_sel_strap) {
+		/* Virtual Phy: Remove Turbo 200Mbit mode */
+		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
+
+		ctl &= ~(1 << 10); // TURBO BIT
+		res =  regmap_write(chip->regmap,
+				    LAN9303_VIRT_SPECIAL_CTRL, ctl);
+	}
+}
+
 static int lan9303_port_enable(struct dsa_switch *ds, int port,
 			       struct phy_device *phy)
 {
@@ -789,6 +821,7 @@ static struct dsa_switch_ops lan9303_switch_ops = {
 	.get_strings = lan9303_get_strings,
 	.phy_read = lan9303_phy_read,
 	.phy_write = lan9303_phy_write,
+	.adjust_link = lan9303_adjust_link,
 	.get_ethtool_stats = lan9303_get_ethtool_stats,
 	.get_sset_count = lan9303_get_sset_count,
 	.port_enable = lan9303_port_enable,
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1697372 — Re: [PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link() method

On Tue, Jul 25, 2017 at 06:15:47PM +0200, Egil Hjelmeland wrote:
> This makes the driver react to device tree "fixed-link" declaration
> on CPU port.
> 
> - turn off autonegotiation
> - force speed 10 or 100 mb/s
> - force duplex mode
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  drivers/net/dsa/lan9303-core.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 0806a0684d55..be6d78f45a5f 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -17,6 +17,7 @@
>  #include <linux/regmap.h>
>  #include <linux/mutex.h>
>  #include <linux/mii.h>
> +#include <linux/phy.h>
>  
>  #include "lan9303.h"
>  
> @@ -746,6 +747,37 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>  	return chip->ops->phy_write(chip, phy, regnum, val);
>  }
>  
> +static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> +				struct phy_device *phydev)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	int ctl, res;
> +
> +	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> +	if (!phy_is_pseudo_fixed_link(phydev))
> +		return;
> +

Hi Egil

Maybe do this check before reading MII_BMCR?

> +	ctl &= ~BMCR_ANENABLE;

Should this also mask out BMCR_SPEED100 and DUPLEX_FULL? Otherwise how
do you select 10/Half if it is already configured for 100/Full?

> +	if (phydev->speed == SPEED_100)
> +		ctl |= BMCR_SPEED100;
> +
> +	if (phydev->duplex == DUPLEX_FULL)
> +		ctl |= BMCR_FULLDPLX;
> +
> +	res =  lan9303_phy_write(ds, port, MII_BMCR, ctl);
> +
> +	if (port == chip->phy_addr_sel_strap) {
> +		/* Virtual Phy: Remove Turbo 200Mbit mode */
> +		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
> +
> +		ctl &= ~(1 << 10); // TURBO BIT

BIT(10), or better still something like BIT(LAN9303_VIRT_SPECIAL_TURBO)

> +		res =  regmap_write(chip->regmap,
> +				    LAN9303_VIRT_SPECIAL_CTRL, ctl);
> +	}
> +}

  Andrew

[toc] | [prev] | [next] | [standalone]

#1697883 — Re: [PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link() method

On 26. juli 2017 19:09, Andrew Lunn wrote:
> On Tue, Jul 25, 2017 at 06:15:47PM +0200, Egil Hjelmeland wrote:
>> This makes the driver react to device tree "fixed-link" declaration
>> on CPU port.
>>
>> - turn off autonegotiation
>> - force speed 10 or 100 mb/s
>> - force duplex mode
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
>> ---
>>   drivers/net/dsa/lan9303-core.c | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index 0806a0684d55..be6d78f45a5f 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/regmap.h>
>>   #include <linux/mutex.h>
>>   #include <linux/mii.h>
>> +#include <linux/phy.h>
>>   
>>   #include "lan9303.h"
>>   
>> @@ -746,6 +747,37 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>>   	return chip->ops->phy_write(chip, phy, regnum, val);
>>   }
>>   
>> +static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>> +				struct phy_device *phydev)
>> +{
>> +	struct lan9303 *chip = ds->priv;
>> +
>> +	int ctl, res;
>> +
>> +	ctl = lan9303_phy_read(ds, port, MII_BMCR);
>> +
>> +	if (!phy_is_pseudo_fixed_link(phydev))
>> +		return;
>> +
> 
> Hi Egil
> 
> Maybe do this check before reading MII_BMCR?
> 

OK

>> +	ctl &= ~BMCR_ANENABLE;
> 
> Should this also mask out BMCR_SPEED100 and DUPLEX_FULL? Otherwise how
> do you select 10/Half if it is already configured for 100/Full?
> 

Yes you are right. I started out with setting ctl to the fixed default
value, and later changed code to start by reading it from HW.


>> +	if (phydev->speed == SPEED_100)
>> +		ctl |= BMCR_SPEED100;
>> +
>> +	if (phydev->duplex == DUPLEX_FULL)
>> +		ctl |= BMCR_FULLDPLX;
>> +
>> +	res =  lan9303_phy_write(ds, port, MII_BMCR, ctl);
>> +
>> +	if (port == chip->phy_addr_sel_strap) {
>> +		/* Virtual Phy: Remove Turbo 200Mbit mode */
>> +		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
>> +
>> +		ctl &= ~(1 << 10); // TURBO BIT
> 
> BIT(10), or better still something like BIT(LAN9303_VIRT_SPECIAL_TURBO)
> 

Agree

>> +		res =  regmap_write(chip->regmap,
>> +				    LAN9303_VIRT_SPECIAL_CTRL, ctl);
>> +	}
>> +}
> 
>    Andrew
> 

[toc] | [prev] | [next] | [standalone]

#1695948 — [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

Fixes after testing on actual HW:

- lan9303_mdio_write()/_read() must multiply register number
  by 4 to get offset

- Indirect access (PMI) to phy register only work in I2C mode. In
  MDIO mode phy registers must be accessed directly. Introduced
  struct lan9303_phy_ops to handle the two modes. Renamed functions
  to clarify.

- lan9303_detect_phy_setup() : Failed MDIO read return 0xffff.
  Handle that.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 42 +++++++++++++++++++++++++++---------------
 drivers/net/dsa/lan9303.h      | 11 +++++++++++
 drivers/net/dsa/lan9303_i2c.c  |  2 ++
 drivers/net/dsa/lan9303_mdio.c | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index cd76e61f1fca..e622db586c3d 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -20,6 +20,9 @@
 
 #include "lan9303.h"
 
+/* 13.2 System Control and Status Registers
+ * Multiply register number by 4 to get address offset.
+ */
 #define LAN9303_CHIP_REV 0x14
 # define LAN9303_CHIP_ID 0x9303
 #define LAN9303_IRQ_CFG 0x15
@@ -53,6 +56,9 @@
 #define LAN9303_VIRT_PHY_BASE 0x70
 #define LAN9303_VIRT_SPECIAL_CTRL 0x77
 
+/*13.4 Switch Fabric Control and Status Registers
+ * Accessed indirectly via SWITCH_CSR_CMD, SWITCH_CSR_DATA.
+ */
 #define LAN9303_SW_DEV_ID 0x0000
 #define LAN9303_SW_RESET 0x0001
 #define LAN9303_SW_RESET_RESET BIT(0)
@@ -242,7 +248,7 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
 	return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val);
 }
 
-static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
+static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
 {
 	int ret, i;
 	u32 reg;
@@ -262,7 +268,7 @@ static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
 	return -EIO;
 }
 
-static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
+static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)
 {
 	int ret;
 	u32 val;
@@ -272,7 +278,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
 
 	mutex_lock(&chip->indirect_mutex);
 
-	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	ret = lan9303_indirect_phy_wait_for_completion(chip);
 	if (ret)
 		goto on_error;
 
@@ -281,7 +287,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
 	if (ret)
 		goto on_error;
 
-	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	ret = lan9303_indirect_phy_wait_for_completion(chip);
 	if (ret)
 		goto on_error;
 
@@ -299,8 +305,8 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
 	return ret;
 }
 
-static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
-				 unsigned int val)
+static int lan9303_indirect_phy_write(struct lan9303 *chip, int addr,
+				      int regnum, u16 val)
 {
 	int ret;
 	u32 reg;
@@ -311,7 +317,7 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
 
 	mutex_lock(&chip->indirect_mutex);
 
-	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	ret = lan9303_indirect_phy_wait_for_completion(chip);
 	if (ret)
 		goto on_error;
 
@@ -328,6 +334,11 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
 	return ret;
 }
 
+const struct lan9303_phy_ops lan9303_indirect_phy_ops = {
+	.phy_read = lan9303_indirect_phy_read,
+	.phy_write = lan9303_indirect_phy_write,
+};
+
 static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
 {
 	int ret, i;
@@ -427,14 +438,15 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 	 * Special reg 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0
 	 * and the IDs are 0-1-2, else it contains something different from
 	 * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3.
+	 * 0xffff is returned for failed MDIO access.
 	 */
-	reg = lan9303_port_phy_reg_read(chip, 3, MII_LAN911X_SPECIAL_MODES);
+	reg = chip->ops->phy_read(chip, 3, MII_LAN911X_SPECIAL_MODES);
 	if (reg < 0) {
 		dev_err(chip->dev, "Failed to detect phy config: %d\n", reg);
 		return reg;
 	}
 
-	if (reg != 0)
+	if ((reg != 0) && (reg != 0xffff))
 		chip->phy_addr_sel_strap = 1;
 	else
 		chip->phy_addr_sel_strap = 0;
@@ -719,7 +731,7 @@ static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
 	if (phy > phy_base + 2)
 		return -ENODEV;
 
-	return lan9303_port_phy_reg_read(chip, phy, regnum);
+	return chip->ops->phy_read(chip, phy, regnum);
 }
 
 static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
@@ -733,7 +745,7 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	if (phy > phy_base + 2)
 		return -ENODEV;
 
-	return lan9303_phy_reg_write(chip, phy, regnum, val);
+	return chip->ops->phy_write(chip, phy, regnum, val);
 }
 
 static int lan9303_port_enable(struct dsa_switch *ds, int port,
@@ -766,13 +778,13 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	switch (port) {
 	case 1:
 		lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
-		lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
-				      MII_BMCR, BMCR_PDOWN);
+		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
+				  MII_BMCR, BMCR_PDOWN);
 		break;
 	case 2:
 		lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
-		lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 2,
-				      MII_BMCR, BMCR_PDOWN);
+		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
+				  MII_BMCR, BMCR_PDOWN);
 		break;
 	default:
 		dev_dbg(chip->dev,
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index d1512dad2d90..444d00b460e1 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -2,6 +2,15 @@
 #include <linux/device.h>
 #include <net/dsa.h>
 
+struct lan9303;
+
+struct lan9303_phy_ops {
+	/* PHY 1 &2 access*/
+	int	(*phy_read)(struct lan9303 *chip, int port, int regnum);
+	int	(*phy_write)(struct lan9303 *chip, int port,
+			     int regnum, u16 val);
+};
+
 struct lan9303 {
 	struct device *dev;
 	struct regmap *regmap;
@@ -11,9 +20,11 @@ struct lan9303 {
 	bool phy_addr_sel_strap;
 	struct dsa_switch *ds;
 	struct mutex indirect_mutex; /* protect indexed register access */
+	const struct lan9303_phy_ops *ops;
 };
 
 extern const struct regmap_access_table lan9303_register_set;
+extern const struct lan9303_phy_ops lan9303_indirect_phy_ops;
 
 int lan9303_probe(struct lan9303 *chip, struct device_node *np);
 int lan9303_remove(struct lan9303 *chip);
diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c
index ab3ce0da5071..24ec20f7f444 100644
--- a/drivers/net/dsa/lan9303_i2c.c
+++ b/drivers/net/dsa/lan9303_i2c.c
@@ -63,6 +63,8 @@ static int lan9303_i2c_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, sw_dev);
 	sw_dev->chip.dev = &client->dev;
 
+	sw_dev->chip.ops = &lan9303_indirect_phy_ops;
+
 	ret = lan9303_probe(&sw_dev->chip, client->dev.of_node);
 	if (ret != 0)
 		return ret;
diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c
index 93c36c0541cf..94df12c5362f 100644
--- a/drivers/net/dsa/lan9303_mdio.c
+++ b/drivers/net/dsa/lan9303_mdio.c
@@ -39,6 +39,7 @@ static void lan9303_mdio_real_write(struct mdio_device *mdio, int reg, u16 val)
 static int lan9303_mdio_write(void *ctx, uint32_t reg, uint32_t val)
 {
 	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
+	reg <<= 2; /* reg num to offset */
 
 	mutex_lock(&sw_dev->device->bus->mdio_lock);
 	lan9303_mdio_real_write(sw_dev->device, reg, val & 0xffff);
@@ -56,6 +57,7 @@ static u16 lan9303_mdio_real_read(struct mdio_device *mdio, int reg)
 static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
 {
 	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
+	reg <<= 2; /* reg num to offset */
 
 	mutex_lock(&sw_dev->device->bus->mdio_lock);
 	*val = lan9303_mdio_real_read(sw_dev->device, reg);
@@ -65,6 +67,36 @@ static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
 	return 0;
 }
 
+int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val)
+{
+	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
+	struct mdio_device *mdio = sw_dev->device;
+
+	mutex_lock(&mdio->bus->mdio_lock);
+	mdio->bus->write(mdio->bus, phy, regnum, val);
+	mutex_unlock(&mdio->bus->mdio_lock);
+
+	return 0;
+}
+
+int lan9303_mdio_phy_read(struct lan9303 *chip, int phy,  int reg)
+{
+	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
+	struct mdio_device *mdio = sw_dev->device;
+	int val;
+
+	mutex_lock(&mdio->bus->mdio_lock);
+	val  =  mdio->bus->read(mdio->bus, phy, reg);
+	mutex_unlock(&mdio->bus->mdio_lock);
+
+	return val;
+}
+
+static const struct lan9303_phy_ops lan9303_mdio_phy_ops = {
+	.phy_read = lan9303_mdio_phy_read,
+	.phy_write = lan9303_mdio_phy_write,
+};
+
 static const struct regmap_config lan9303_mdio_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 32,
@@ -106,6 +138,8 @@ static int lan9303_mdio_probe(struct mdio_device *mdiodev)
 	dev_set_drvdata(&mdiodev->dev, sw_dev);
 	sw_dev->chip.dev = &mdiodev->dev;
 
+	sw_dev->chip.ops = &lan9303_mdio_phy_ops;
+
 	ret = lan9303_probe(&sw_dev->chip, mdiodev->dev.of_node);
 	if (ret != 0)
 		return ret;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1696049 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

> Fixes after testing on actual HW:
>
> - lan9303_mdio_write()/_read() must multiply register number
>   by 4 to get offset
>
> - Indirect access (PMI) to phy register only work in I2C mode. In
>   MDIO mode phy registers must be accessed directly. Introduced
>   struct lan9303_phy_ops to handle the two modes. Renamed functions
>   to clarify.
>
> - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff.
>   Handle that.

Small patch series when possible are better. Bullet points in commit
messages are likely to describe how a patch or series may be split up
;-)

This patch seems to be the unique patch of the series resolving what is
described in the cover letter as "Make the MDIO interface work".

I'd suggest you to split up this one commit in several *atomic* and easy
to review patches and send them separately as on thread named "net: dsa:
lan9303: fix MDIO interface" (also note that imperative is prefered for
subject lines, see: https://chris.beams.io/posts/git-commit/#imperative)

<...>

> -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
> +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)

For instance you can have a first commit only renaming the functions.
The reason for it is to separate the functional changes from cosmetic
changes, which makes it easier for review.

<...>

> -	if (reg != 0)
> +	if ((reg != 0) && (reg != 0xffff))

if (reg && reg != 0xffff) should be enough.

>  		chip->phy_addr_sel_strap = 1;
>  	else
>  		chip->phy_addr_sel_strap = 0;

<...>

> +struct lan9303;
> +
> +struct lan9303_phy_ops {
> +	/* PHY 1 &2 access*/

The spacing is weird in the comment. "/* PHY 1 & 2 access */" maybe?

<...>

> +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val)
> +{
> +	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
> +	struct mdio_device *mdio = sw_dev->device;
> +
> +	mutex_lock(&mdio->bus->mdio_lock);
> +	mdio->bus->write(mdio->bus, phy, regnum, val);
> +	mutex_unlock(&mdio->bus->mdio_lock);

This is exactly what mdiobus_write(mdio->bus, phy, regnum, val) is
doing. There are very few valid reasons to go play in the mii_bus
structure, using generic APIs are strongly prefered. Plus you have
checks and traces for free!

> +
> +	return 0;
> +}
> +
> +int lan9303_mdio_phy_read(struct lan9303 *chip, int phy,  int reg)
> +{
> +	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
> +	struct mdio_device *mdio = sw_dev->device;
> +	int val;
> +
> +	mutex_lock(&mdio->bus->mdio_lock);
> +	val  =  mdio->bus->read(mdio->bus, phy, reg);
> +	mutex_unlock(&mdio->bus->mdio_lock);

Same here, mdiobus_read().


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1697040 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

On 25. juli 2017 21:15, Vivien Didelot wrote:
> Hi Egil,
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>> Fixes after testing on actual HW:
>>
>> - lan9303_mdio_write()/_read() must multiply register number
>>    by 4 to get offset
>>
>> - Indirect access (PMI) to phy register only work in I2C mode. In
>>    MDIO mode phy registers must be accessed directly. Introduced
>>    struct lan9303_phy_ops to handle the two modes. Renamed functions
>>    to clarify.
>>
>> - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff.
>>    Handle that.
> 
> Small patch series when possible are better. Bullet points in commit
> messages are likely to describe how a patch or series may be split up
> ;-)
> 
> This patch seems to be the unique patch of the series resolving what is
> described in the cover letter as "Make the MDIO interface work".
> 
> I'd suggest you to split up this one commit in several *atomic* and easy
> to review patches and send them separately as on thread named "net: dsa:
> lan9303: fix MDIO interface" (also note that imperative is prefered for
> subject lines, see: https://chris.beams.io/posts/git-commit/#imperative)
> 
> <...>
> 
>> -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
>> +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
> 
> For instance you can have a first commit only renaming the functions.
> The reason for it is to separate the functional changes from cosmetic
> changes, which makes it easier for review.
> 
> <...>

Thank you for reviewing.

I can split the first patch.

I can also split the patch series to more digestible series. But
since most of the patches touches the same file, I assume that each
series must be completed and applied before starting on a new one.
So I really want to group the patches into only a few series in order
to not spend months on the process.


>> +	if ((reg != 0) && (reg != 0xffff))
> 
> if (reg && reg != 0xffff) should be enough.

Of course.

>> +struct lan9303_phy_ops {
>> +	/* PHY 1 &2 access*/
> 
> The spacing is weird in the comment. "/* PHY 1 & 2 access */" maybe?
> 

Yes.

>> +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val)
>> +{
>> +	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
>> +	struct mdio_device *mdio = sw_dev->device;
>> +
>> +	mutex_lock(&mdio->bus->mdio_lock);
>> +	mdio->bus->write(mdio->bus, phy, regnum, val);
>> +	mutex_unlock(&mdio->bus->mdio_lock);
> 
> This is exactly what mdiobus_write(mdio->bus, phy, regnum, val) is
> doing. There are very few valid reasons to go play in the mii_bus
> structure, using generic APIs are strongly prefered. Plus you have
> checks and traces for free!
> 

Lack of oversight was the only reason. I just adapted stuff from
lan9303_mdio_phy_write above. Will switch to mdiobus_write of course.

> Same here, mdiobus_read().
> 
Ditto.

> 
> Thanks,
> 
>          Vivien
> 

Appreciated,
Egil

[toc] | [prev] | [next] | [standalone]

#1697213 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

>> I'd suggest you to split up this one commit in several *atomic* and easy
>> to review patches and send them separately as on thread named "net: dsa:
>> lan9303: fix MDIO interface" (also note that imperative is prefered for
>> subject lines, see: https://chris.beams.io/posts/git-commit/#imperative)
>
> I can split the first patch.
>
> I can also split the patch series to more digestible series. But
> since most of the patches touches the same file, I assume that each
> series must be completed and applied before starting on a new one.
> So I really want to group the patches into only a few series in order
> to not spend months on the process.

I understand. But believe me, your patches are very likely to land
mainline faster if you send them in small chunks. This might not be true
for every subsystems, but netdev is very responsive. This is even more
true since this series has no-no (such as the sysfs entries) which
guarantees the whole patch series to be rejected.

Sending portions of your local work branch then rebase it against
net-next/master is a usual development process.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1697263 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

On 26. juli 2017 16:30, Vivien Didelot wrote:
> Hi Egil,
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>>> I'd suggest you to split up this one commit in several *atomic* and easy
>>> to review patches and send them separately as on thread named "net: dsa:
>>> lan9303: fix MDIO interface" (also note that imperative is prefered for
>>> subject lines, see: https://chris.beams.io/posts/git-commit/#imperative)
>>
>> I can split the first patch.
>>
>> I can also split the patch series to more digestible series. But
>> since most of the patches touches the same file, I assume that each
>> series must be completed and applied before starting on a new one.
>> So I really want to group the patches into only a few series in order
>> to not spend months on the process.
> 
> I understand. But believe me, your patches are very likely to land
> mainline faster if you send them in small chunks. This might not be true
> for every subsystems, but netdev is very responsive. This is even more
> true since this series has no-no (such as the sysfs entries) which
> guarantees the whole patch series to be rejected.
> 
> Sending portions of your local work branch then rebase it against
> net-next/master is a usual development process.
> 
> 
> Thanks,
> 
>          Vivien
> 

Thank you for the advice. I got some other NMIs today that I have to
serve. Hope to come back with MDIO patch series soon.

Egil

[toc] | [prev] | [next] | [standalone]

#1697461 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

> > So I really want to group the patches into only a few series in order
> > to not spend months on the process.

I strongly agree with Vivien here. Good patches get accepted in about
3 days. You should expect feedback within a day or two. That allows
you to have fast cycle times for getting patches in.

    Andrew

[toc] | [prev] | [next] | [standalone]

#1697534 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 26 Jul 2017 19:52:24 +0200

>> > So I really want to group the patches into only a few series in order
>> > to not spend months on the process.
> 
> I strongly agree with Vivien here. Good patches get accepted in about
> 3 days. You should expect feedback within a day or two. That allows
> you to have fast cycle times for getting patches in.

+1

Small simple patches will get everything in 10 times fast than if
you clump everything together into larger, harder to review ones.

[toc] | [prev] | [next] | [standalone]

#1697554 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

Den 26. juli 2017 22:07, skrev David Miller:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Wed, 26 Jul 2017 19:52:24 +0200
> 
>>>> So I really want to group the patches into only a few series in order
>>>> to not spend months on the process.
>>
>> I strongly agree with Vivien here. Good patches get accepted in about
>> 3 days. You should expect feedback within a day or two. That allows
>> you to have fast cycle times for getting patches in.
> 
> +1
> 
> Small simple patches will get everything in 10 times fast than if
> you clump everything together into larger, harder to review ones.
> 

Good. Just one question about process. Could I have posted my work
as a RFC? To get one round of initial feedback before chopping into
small patch requests. As well as indicating where I am heading. Or is
that just waste of human bandwidth?

Egil

[toc] | [prev] | [next] | [standalone]

#1697588 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

> Good. Just one question about process. Could I have posted my work
> as a RFC? To get one round of initial feedback before chopping into
> small patch requests. As well as indicating where I am heading. Or is
> that just waste of human bandwidth?

Depends. Post 100 RFC patches, i won't look at them. Post 21 with a
cover note making it clear you are planning to submit them in blocks
of 7, i might.

But it is best to assume reviewers have small blocks of time. 21
patches take 3 times a long to review as 7. The block of time might
not be enough for 21, so the review gets differed. 7 are more likely
to fit in the available time, so it happens quickly.

   Andrew

[toc] | [prev] | [next] | [standalone]

#1697359 — Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

On Tue, Jul 25, 2017 at 06:15:44PM +0200, Egil Hjelmeland wrote:
> Fixes after testing on actual HW:
> 
> - lan9303_mdio_write()/_read() must multiply register number
>   by 4 to get offset
> 
> - Indirect access (PMI) to phy register only work in I2C mode. In
>   MDIO mode phy registers must be accessed directly. Introduced
>   struct lan9303_phy_ops to handle the two modes. Renamed functions
>   to clarify.
> 
> - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff.
>   Handle that.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  drivers/net/dsa/lan9303-core.c | 42 +++++++++++++++++++++++++++---------------
>  drivers/net/dsa/lan9303.h      | 11 +++++++++++
>  drivers/net/dsa/lan9303_i2c.c  |  2 ++
>  drivers/net/dsa/lan9303_mdio.c | 34 ++++++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index cd76e61f1fca..e622db586c3d 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -20,6 +20,9 @@
>  
>  #include "lan9303.h"
>  
> +/* 13.2 System Control and Status Registers
> + * Multiply register number by 4 to get address offset.
> + */
>  #define LAN9303_CHIP_REV 0x14
>  # define LAN9303_CHIP_ID 0x9303
>  #define LAN9303_IRQ_CFG 0x15
> @@ -53,6 +56,9 @@
>  #define LAN9303_VIRT_PHY_BASE 0x70
>  #define LAN9303_VIRT_SPECIAL_CTRL 0x77
>  
> +/*13.4 Switch Fabric Control and Status Registers
> + * Accessed indirectly via SWITCH_CSR_CMD, SWITCH_CSR_DATA.
> + */
>  #define LAN9303_SW_DEV_ID 0x0000
>  #define LAN9303_SW_RESET 0x0001
>  #define LAN9303_SW_RESET_RESET BIT(0)
> @@ -242,7 +248,7 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
>  	return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val);
>  }
>  
> -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
> +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
>  {
>  	int ret, i;
>  	u32 reg;
> @@ -262,7 +268,7 @@ static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
>  	return -EIO;
>  }
>  
> -static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
> +static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)
>  {
>  	int ret;
>  	u32 val;
> @@ -272,7 +278,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
>  
>  	mutex_lock(&chip->indirect_mutex);
>  
> -	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	ret = lan9303_indirect_phy_wait_for_completion(chip);
>  	if (ret)
>  		goto on_error;
>  
> @@ -281,7 +287,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
>  	if (ret)
>  		goto on_error;
>  
> -	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	ret = lan9303_indirect_phy_wait_for_completion(chip);
>  	if (ret)
>  		goto on_error;
>  
> @@ -299,8 +305,8 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
>  	return ret;
>  }
>  
> -static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
> -				 unsigned int val)
> +static int lan9303_indirect_phy_write(struct lan9303 *chip, int addr,
> +				      int regnum, u16 val)
>  {
>  	int ret;
>  	u32 reg;
> @@ -311,7 +317,7 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
>  
>  	mutex_lock(&chip->indirect_mutex);
>  
> -	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	ret = lan9303_indirect_phy_wait_for_completion(chip);
>  	if (ret)
>  		goto on_error;
>  
> @@ -328,6 +334,11 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
>  	return ret;
>  }
>  
> +const struct lan9303_phy_ops lan9303_indirect_phy_ops = {
> +	.phy_read = lan9303_indirect_phy_read,
> +	.phy_write = lan9303_indirect_phy_write,
> +};
> +
>  static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
>  {
>  	int ret, i;
> @@ -427,14 +438,15 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
>  	 * Special reg 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0
>  	 * and the IDs are 0-1-2, else it contains something different from
>  	 * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3.
> +	 * 0xffff is returned for failed MDIO access.

Hi Egil

0xffff is not really a failure. It just means there is nothing on the
bus at that address. The bus has a weak pull-up, so defaults to one.

>  	 */
> -	reg = lan9303_port_phy_reg_read(chip, 3, MII_LAN911X_SPECIAL_MODES);
> +	reg = chip->ops->phy_read(chip, 3, MII_LAN911X_SPECIAL_MODES);
>  	if (reg < 0) {
>  		dev_err(chip->dev, "Failed to detect phy config: %d\n", reg);
>  		return reg;
>  	}
>  
> -	if (reg != 0)
> +	if ((reg != 0) && (reg != 0xffff))
>  		chip->phy_addr_sel_strap = 1;
>  	else
>  		chip->phy_addr_sel_strap = 0;
> @@ -719,7 +731,7 @@ static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
>  	if (phy > phy_base + 2)
>  		return -ENODEV;
>  
> -	return lan9303_port_phy_reg_read(chip, phy, regnum);
> +	return chip->ops->phy_read(chip, phy, regnum);
>  }
>  
>  static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
> @@ -733,7 +745,7 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>  	if (phy > phy_base + 2)
>  		return -ENODEV;
>  
> -	return lan9303_phy_reg_write(chip, phy, regnum, val);
> +	return chip->ops->phy_write(chip, phy, regnum, val);
>  }
>  
>  static int lan9303_port_enable(struct dsa_switch *ds, int port,
> @@ -766,13 +778,13 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>  	switch (port) {
>  	case 1:
>  		lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
> -		lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
> -				      MII_BMCR, BMCR_PDOWN);
> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
> +				  MII_BMCR, BMCR_PDOWN);
>  		break;
>  	case 2:
>  		lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
> -		lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 2,
> -				      MII_BMCR, BMCR_PDOWN);
> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
> +				  MII_BMCR, BMCR_PDOWN);
>  		break;
>  	default:
>  		dev_dbg(chip->dev,
> diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
> index d1512dad2d90..444d00b460e1 100644
> --- a/drivers/net/dsa/lan9303.h
> +++ b/drivers/net/dsa/lan9303.h
> @@ -2,6 +2,15 @@
>  #include <linux/device.h>
>  #include <net/dsa.h>
>  
> +struct lan9303;
> +
> +struct lan9303_phy_ops {
> +	/* PHY 1 &2 access*/

A space would be nice here after the &.

> +	int	(*phy_read)(struct lan9303 *chip, int port, int regnum);
> +	int	(*phy_write)(struct lan9303 *chip, int port,
> +			     int regnum, u16 val);
> +};
> +
>  struct lan9303 {
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -11,9 +20,11 @@ struct lan9303 {
>  	bool phy_addr_sel_strap;
>  	struct dsa_switch *ds;
>  	struct mutex indirect_mutex; /* protect indexed register access */
> +	const struct lan9303_phy_ops *ops;
>  };
>  
>  extern const struct regmap_access_table lan9303_register_set;
> +extern const struct lan9303_phy_ops lan9303_indirect_phy_ops;
>  
>  int lan9303_probe(struct lan9303 *chip, struct device_node *np);
>  int lan9303_remove(struct lan9303 *chip);
> diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c
> index ab3ce0da5071..24ec20f7f444 100644
> --- a/drivers/net/dsa/lan9303_i2c.c
> +++ b/drivers/net/dsa/lan9303_i2c.c
> @@ -63,6 +63,8 @@ static int lan9303_i2c_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, sw_dev);
>  	sw_dev->chip.dev = &client->dev;
>  
> +	sw_dev->chip.ops = &lan9303_indirect_phy_ops;
> +
>  	ret = lan9303_probe(&sw_dev->chip, client->dev.of_node);
>  	if (ret != 0)
>  		return ret;
> diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c
> index 93c36c0541cf..94df12c5362f 100644
> --- a/drivers/net/dsa/lan9303_mdio.c
> +++ b/drivers/net/dsa/lan9303_mdio.c
> @@ -39,6 +39,7 @@ static void lan9303_mdio_real_write(struct mdio_device *mdio, int reg, u16 val)
>  static int lan9303_mdio_write(void *ctx, uint32_t reg, uint32_t val)
>  {
>  	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
> +	reg <<= 2; /* reg num to offset */
>  
>  	mutex_lock(&sw_dev->device->bus->mdio_lock);
>  	lan9303_mdio_real_write(sw_dev->device, reg, val & 0xffff);
> @@ -56,6 +57,7 @@ static u16 lan9303_mdio_real_read(struct mdio_device *mdio, int reg)
>  static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
>  {
>  	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
> +	reg <<= 2; /* reg num to offset */
>  
>  	mutex_lock(&sw_dev->device->bus->mdio_lock);
>  	*val = lan9303_mdio_real_read(sw_dev->device, reg);
> @@ -65,6 +67,36 @@ static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
>  	return 0;
>  }
>  
> +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val)
> +{
> +	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
> +	struct mdio_device *mdio = sw_dev->device;
> +
> +	mutex_lock(&mdio->bus->mdio_lock);
> +	mdio->bus->write(mdio->bus, phy, regnum, val);
> +	mutex_unlock(&mdio->bus->mdio_lock);

It is better to use mdiobus_read/write or if you are nesting mdio
busses, mdiobus_read_nested/mdiobus_write_nested. Please test this
code with lockdep enabled.

And as others have pointed out, there are too many changes in this one
patch.

Thanks
	Andrew

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web