2019-10-14 06:17:32

by Oleksij Rempel

[permalink] [raw]
Subject: [PATCH v1 0/4] add dsa switch support for ar9331

This patch series provides dsa switch support for Atheros ar9331 WiSoC.
As side effect ag71xx needed to be ported to phylink to make the switch
driver (as well phylink based) work properly.

Oleksij Rempel (4):
net: ag71xx: port to phylink
dt-bindings: net: dsa: qca,ar9331 switch documentation
MIPS: ath79: ar9331: add ar9331-switch node
net: dsa: add support for Atheros AR9331 build-in switch

.../devicetree/bindings/net/dsa/ar9331.txt | 155 ++++
arch/mips/boot/dts/qca/ar9331.dtsi | 128 ++-
arch/mips/boot/dts/qca/ar9331_dpt_module.dts | 13 +
drivers/net/dsa/Kconfig | 2 +
drivers/net/dsa/Makefile | 1 +
drivers/net/dsa/qca/Kconfig | 11 +
drivers/net/dsa/qca/Makefile | 2 +
drivers/net/dsa/qca/ar9331.c | 822 ++++++++++++++++++
drivers/net/ethernet/atheros/Kconfig | 2 +-
drivers/net/ethernet/atheros/ag71xx.c | 144 +--
include/net/dsa.h | 2 +
net/dsa/Kconfig | 6 +
net/dsa/Makefile | 1 +
net/dsa/tag_ar9331.c | 97 +++
14 files changed, 1324 insertions(+), 62 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/dsa/ar9331.txt
create mode 100644 drivers/net/dsa/qca/Kconfig
create mode 100644 drivers/net/dsa/qca/Makefile
create mode 100644 drivers/net/dsa/qca/ar9331.c
create mode 100644 net/dsa/tag_ar9331.c

--
2.23.0


2019-10-14 06:17:53

by Oleksij Rempel

[permalink] [raw]
Subject: [PATCH v1 4/4] net: dsa: add support for Atheros AR9331 build-in switch

Provide basic support for Atheros AR9331 build-in switch. So far it
works as port multiplexer without any hardware offloading support.

Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/Kconfig | 2 +
drivers/net/dsa/Makefile | 1 +
drivers/net/dsa/qca/Kconfig | 11 +
drivers/net/dsa/qca/Makefile | 2 +
drivers/net/dsa/qca/ar9331.c | 822 +++++++++++++++++++++++++++++++++++
include/net/dsa.h | 2 +
net/dsa/Kconfig | 6 +
net/dsa/Makefile | 1 +
net/dsa/tag_ar9331.c | 97 +++++
9 files changed, 944 insertions(+)
create mode 100644 drivers/net/dsa/qca/Kconfig
create mode 100644 drivers/net/dsa/qca/Makefile
create mode 100644 drivers/net/dsa/qca/ar9331.c
create mode 100644 net/dsa/tag_ar9331.c

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index f6232ce8481f..de9610a3dd5c 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -52,6 +52,8 @@ source "drivers/net/dsa/microchip/Kconfig"

source "drivers/net/dsa/mv88e6xxx/Kconfig"

+source "drivers/net/dsa/qca/Kconfig"
+
source "drivers/net/dsa/sja1105/Kconfig"

config NET_DSA_QCA8K
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index ae70b79628d6..90e5b1ed69c5 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -20,4 +20,5 @@ obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
obj-y += b53/
obj-y += microchip/
obj-y += mv88e6xxx/
+obj-y += qca/
obj-y += sja1105/
diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
new file mode 100644
index 000000000000..7e4978f46642
--- /dev/null
+++ b/drivers/net/dsa/qca/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config NET_DSA_AR9331
+ tristate "Atheros AR9331 Ethernet switch support"
+ depends on NET_DSA
+ select NET_DSA_TAG_AR9331
+ select REGMAP
+ ---help---
+ This enables support for the Atheros AR9331 build-in Ethernet
+ switch.
+
+
diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
new file mode 100644
index 000000000000..274022319066
--- /dev/null
+++ b/drivers/net/dsa/qca/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
new file mode 100644
index 000000000000..62537fef59a7
--- /dev/null
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -0,0 +1,822 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (c) 2019 Pengutronix, Oleksij Rempel <[email protected]>
+
+#include <linux/bitfield.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/of_mdio.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <net/dsa.h>
+
+#define AR9331_SW_NAME "ar9331_switch"
+#define AR9331_SW_PORTS 6
+
+/* dummy reg to change page */
+#define AR9331_SW_REG_PAGE BIT(18)
+
+/* Global Interrupt */
+#define AR9331_SW_REG_GINT 0x10
+#define AR9331_SW_REG_GINT_MASK 0x14
+#define AR9331_SW_GINT_PHY_INT BIT(2)
+
+#define AR9331_SW_REG_FLOOD_MASK 0x2c
+#define AR9331_SW_FLOOD_MASK_BROAD_TO_CPU BIT(26)
+
+#define AR9331_SW_REG_GLOBAL_CTRL 0x30
+#define AR9331_SW_GLOBAL_CTRL_MFS_M GENMASK(13, 0)
+
+#define AR9331_SW_REG_MDIO_CTRL 0x98
+#define AR9331_SW_MDIO_CTRL_BUSY BIT(31)
+#define AR9331_SW_MDIO_CTRL_MASTER_EN BIT(30)
+#define AR9331_SW_MDIO_CTRL_CMD_READ BIT(27)
+#define AR9331_SW_MDIO_CTRL_PHY_ADDR_M GENMASK(25, 21)
+#define AR9331_SW_MDIO_CTRL_REG_ADDR_M GENMASK(20, 16)
+#define AR9331_SW_MDIO_CTRL_DATA_M GENMASK(16, 0)
+
+#define AR9331_SW_REG_PORT_STATUS(_port) (0x100 + (_port) * 0x100)
+
+/* FLOW_LINK_EN - enable mac flow control config auto-neg with phy.
+ * If not set, mac can be config by software.
+ */
+#define AR9331_SW_PORT_STATUS_FLOW_LINK_EN BIT(12)
+
+/* LINK_EN - If set, MAC is configured from PHY link status.
+ * If not set, MAC should be configured by software.
+ */
+#define AR9331_SW_PORT_STATUS_LINK_EN BIT(9)
+#define AR9331_SW_PORT_STATUS_DUPLEX_MODE BIT(6)
+#define AR9331_SW_PORT_STATUS_RX_FLOW_EN BIT(5)
+#define AR9331_SW_PORT_STATUS_TX_FLOW_EN BIT(4)
+#define AR9331_SW_PORT_STATUS_RXMAC BIT(3)
+#define AR9331_SW_PORT_STATUS_TXMAC BIT(2)
+#define AR9331_SW_PORT_STATUS_SPEED_M GENMASK(1, 0)
+#define AR9331_SW_PORT_STATUS_SPEED_1000 2
+#define AR9331_SW_PORT_STATUS_SPEED_100 1
+#define AR9331_SW_PORT_STATUS_SPEED_10 0
+
+#define AR9331_SW_PORT_STATUS_MAC_MASK \
+ (AR9331_SW_PORT_STATUS_TXMAC | AR9331_SW_PORT_STATUS_RXMAC)
+
+#define AR9331_SW_PORT_STATUS_LINK_MASK \
+ (AR9331_SW_PORT_STATUS_LINK_EN | AR9331_SW_PORT_STATUS_FLOW_LINK_EN | \
+ AR9331_SW_PORT_STATUS_DUPLEX_MODE | \
+ AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
+ AR9331_SW_PORT_STATUS_SPEED_M)
+
+/* Phy bypass mode
+ * ------------------------------------------------------------------------
+ * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
+ *
+ * real | start | OP | PhyAddr | Reg Addr | TA |
+ * atheros| start | OP | 2'b00 |PhyAdd[2:0]| Reg Addr[4:0] | TA |
+ *
+ *
+ * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
+ * real | Data |
+ * atheros| Data |
+ *
+ * ------------------------------------------------------------------------
+ * Page address mode
+ * ------------------------------------------------------------------------
+ * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
+ * real | start | OP | PhyAddr | Reg Addr | TA |
+ * atheros| start | OP | 2'b11 | 8'b0 | TA |
+ *
+ * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
+ * real | Data |
+ * atheros| | Page [9:0] |
+ */
+/* In case of Page Address mode, Bit[18:9] of 32 bit register address should be
+ * written to bits[9:0] of mdio data register.
+ */
+#define AR9331_SW_ADDR_PAGE GENMASK(18, 9)
+
+/* ------------------------------------------------------------------------
+ * Normal register access mode
+ * ------------------------------------------------------------------------
+ * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
+ * real | start | OP | PhyAddr | Reg Addr | TA |
+ * atheros| start | OP | 2'b10 | low_addr[7:0] | TA |
+ *
+ * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
+ * real | Data |
+ * atheros| Data |
+ * ------------------------------------------------------------------------
+ */
+#define AR9331_SW_LOW_ADDR_PHY GENMASK(8, 6)
+#define AR9331_SW_LOW_ADDR_REG GENMASK(5, 1)
+
+#define AR9331_SW_MDIO_PHY_MODE_M GENMASK(4, 3)
+#define AR9331_SW_MDIO_PHY_MODE_PAGE 3
+#define AR9331_SW_MDIO_PHY_MODE_REG 2
+#define AR9331_SW_MDIO_PHY_MODE_BYPASS 0
+#define AR9331_SW_MDIO_PHY_ADDR_M GENMASK(2, 0)
+
+/* Empirical determined values */
+#define AR9331_SW_MDIO_POLL_SLEEP_US 1
+#define AR9331_SW_MDIO_POLL_TIMEOUT_US 20
+
+struct ar9331_sw_priv {
+ struct device *dev;
+ struct dsa_switch *ds;
+ struct dsa_switch_ops ops;
+ struct irq_domain *irqdomain;
+ struct mii_bus *mbus; /* mdio master */
+ struct mii_bus *sbus; /* mdio slave */
+ struct regmap *regmap;
+ struct reset_control *sw_reset;
+};
+
+/* Warning: switch reset will reset last AR9331_SW_MDIO_PHY_MODE_PAGE request
+ * If some kind of optimization is used, the request should be repeated.
+ */
+static int ar9331_sw_reset(struct ar9331_sw_priv *priv)
+{
+ int ret;
+
+ ret = reset_control_assert(priv->sw_reset);
+ if (ret)
+ goto error;
+
+ /* AR9331 doc do not provide any information about proper reset
+ * sequence. The AR8136 (the closes switch to the AR9331) doc says:
+ * reset duration should be greater than 10ms. So, let's use this value
+ * for now.
+ */
+ usleep_range(10000, 15000);
+ ret = reset_control_deassert(priv->sw_reset);
+ if (ret)
+ goto error;
+
+ return 0;
+error:
+ dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+ return ret;
+}
+
+static int ar9331_sw_mbus_write(struct mii_bus *mbus, int port, int regnum,
+ u16 data)
+{
+ struct ar9331_sw_priv *priv = mbus->priv;
+ struct regmap *regmap = priv->regmap;
+ u32 val;
+ int ret;
+
+ ret = regmap_write(regmap, AR9331_SW_REG_MDIO_CTRL,
+ AR9331_SW_MDIO_CTRL_BUSY |
+ AR9331_SW_MDIO_CTRL_MASTER_EN |
+ FIELD_PREP(AR9331_SW_MDIO_CTRL_PHY_ADDR_M, port) |
+ FIELD_PREP(AR9331_SW_MDIO_CTRL_REG_ADDR_M, regnum) |
+ FIELD_PREP(AR9331_SW_MDIO_CTRL_DATA_M, data));
+ if (ret)
+ goto error;
+
+ ret = regmap_read_poll_timeout(regmap, AR9331_SW_REG_MDIO_CTRL, val,
+ !(val & AR9331_SW_MDIO_CTRL_BUSY),
+ AR9331_SW_MDIO_POLL_SLEEP_US,
+ AR9331_SW_MDIO_POLL_TIMEOUT_US);
+ if (ret)
+ goto error;
+
+ return 0;
+error:
+ dev_err_ratelimited(priv->dev, "PHY write error: %i\n", ret);
+ return ret;
+}
+
+static int ar9331_sw_mbus_read(struct mii_bus *mbus, int port, int regnum)
+{
+ struct ar9331_sw_priv *priv = mbus->priv;
+ struct regmap *regmap = priv->regmap;
+ u32 val;
+ int ret;
+
+ ret = regmap_write(regmap, AR9331_SW_REG_MDIO_CTRL,
+ AR9331_SW_MDIO_CTRL_BUSY |
+ AR9331_SW_MDIO_CTRL_MASTER_EN |
+ AR9331_SW_MDIO_CTRL_CMD_READ |
+ FIELD_PREP(AR9331_SW_MDIO_CTRL_PHY_ADDR_M, port) |
+ FIELD_PREP(AR9331_SW_MDIO_CTRL_REG_ADDR_M, regnum));
+ if (ret)
+ goto error;
+
+ ret = regmap_read_poll_timeout(regmap, AR9331_SW_REG_MDIO_CTRL, val,
+ !(val & AR9331_SW_MDIO_CTRL_BUSY),
+ AR9331_SW_MDIO_POLL_SLEEP_US,
+ AR9331_SW_MDIO_POLL_TIMEOUT_US);
+ if (ret)
+ goto error;
+
+ ret = regmap_read(regmap, AR9331_SW_REG_MDIO_CTRL, &val);
+ if (ret)
+ goto error;
+
+ return FIELD_GET(AR9331_SW_MDIO_CTRL_DATA_M, val);
+
+error:
+ dev_err_ratelimited(priv->dev, "PHY read error: %i\n", ret);
+ return ret;
+}
+
+static int ar9331_sw_mbus_init(struct ar9331_sw_priv *priv)
+{
+ struct device *dev = priv->dev;
+ static struct mii_bus *mbus;
+ struct device_node *np, *mnp;
+ int ret;
+
+ np = dev->of_node;
+
+ mbus = devm_mdiobus_alloc(dev);
+ if (!mbus)
+ return -ENOMEM;
+
+ mbus->name = np->full_name;
+ snprintf(mbus->id, MII_BUS_ID_SIZE, "%pOF", np);
+
+ mbus->read = ar9331_sw_mbus_read;
+ mbus->write = ar9331_sw_mbus_write;
+ mbus->priv = priv;
+ mbus->parent = dev;
+
+ mnp = of_get_child_by_name(np, "mdio");
+ ret = of_mdiobus_register(mbus, mnp);
+ of_node_put(mnp);
+ if (ret)
+ return ret;
+
+ priv->mbus = mbus;
+
+ return 0;
+}
+
+static int ar9331_sw_setup(struct dsa_switch *ds)
+{
+ struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+ struct regmap *regmap = priv->regmap;
+ int ret;
+
+ ret = ar9331_sw_reset(priv);
+ if (ret)
+ return ret;
+
+ /* Reset will set proper defaults. CPU - Port0 will be enabled and
+ * configured. All other ports (ports 1 - 5) are disabled
+ */
+ ret = ar9331_sw_mbus_init(priv);
+ if (ret)
+ return ret;
+
+ /* Do not drop broadcast frames */
+ ret = regmap_write_bits(regmap, AR9331_SW_REG_FLOOD_MASK,
+ AR9331_SW_FLOOD_MASK_BROAD_TO_CPU,
+ AR9331_SW_FLOOD_MASK_BROAD_TO_CPU);
+ if (ret)
+ goto error;
+
+ /* Sync max frame size with value used in
+ * drivers/net/ethernet/atheros/ag71xx.c for ar9330 SoC.
+ * TODO: In both drivers this value seems to be not real maximal size
+ * The switch is able to configure 0x3fff and ethernet controller
+ * 0xffff. Are there any better way to sync this values?
+ */
+ ret = regmap_write_bits(regmap, AR9331_SW_REG_GLOBAL_CTRL,
+ AR9331_SW_GLOBAL_CTRL_MFS_M,
+ FIELD_PREP(AR9331_SW_GLOBAL_CTRL_MFS_M, 1540));
+ if (ret)
+ goto error;
+
+ return 0;
+error:
+ dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+ return ret;
+}
+
+static int ar9331_sw_port_enable(struct dsa_switch *ds, int port,
+ struct phy_device *phy)
+{
+ struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+ struct regmap *regmap = priv->regmap;
+ int ret;
+
+ /* nothing to enable. Just set link to initial state */
+ ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
+ if (ret)
+ dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+
+ return ret;
+}
+
+static void ar9331_sw_port_disable(struct dsa_switch *ds, int port)
+{
+ struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+ struct regmap *regmap = priv->regmap;
+ int ret;
+
+ ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
+ if (ret)
+ dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+}
+
+static enum dsa_tag_protocol ar9331_sw_get_tag_protocol(struct dsa_switch *ds,
+ int port)
+{
+ return DSA_TAG_PROTO_AR9331;
+}
+
+static void ar9331_sw_phylink_validate(struct dsa_switch *ds, int port,
+ unsigned long *supported,
+ struct phylink_link_state *state)
+{
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+ switch (port) {
+ case 0:
+ if (state->interface != PHY_INTERFACE_MODE_GMII)
+ goto unsupported;
+
+ phylink_set(mask, 1000baseT_Full);
+ phylink_set(mask, 1000baseT_Half);
+ break;
+ case 1:
+ case 2:
+ case 3:
+ case 4:
+ case 5:
+ if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
+ goto unsupported;
+ break;
+ default:
+ bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ dev_err(ds->dev, "Unsupported port: %i\n", port);
+ return;
+ }
+
+ phylink_set_port_modes(mask);
+ phylink_set(mask, Pause);
+ phylink_set(mask, Asym_Pause);
+
+ phylink_set(mask, 10baseT_Half);
+ phylink_set(mask, 10baseT_Full);
+ phylink_set(mask, 100baseT_Half);
+ phylink_set(mask, 100baseT_Full);
+
+ bitmap_and(supported, supported, mask,
+ __ETHTOOL_LINK_MODE_MASK_NBITS);
+ bitmap_and(state->advertising, state->advertising, mask,
+ __ETHTOOL_LINK_MODE_MASK_NBITS);
+
+ return;
+
+unsupported:
+ bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ dev_err(ds->dev, "Unsupported interface: %d, port: %d\n",
+ state->interface, port);
+}
+
+static void ar9331_sw_phylink_mac_config(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ const struct phylink_link_state *state)
+{
+ struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+ struct regmap *regmap = priv->regmap;
+ int ret;
+ u32 val;
+
+ switch (state->speed) {
+ case SPEED_1000:
+ val = AR9331_SW_PORT_STATUS_SPEED_1000;
+ break;
+ case SPEED_100:
+ val = AR9331_SW_PORT_STATUS_SPEED_100;
+ break;
+ case SPEED_10:
+ val = AR9331_SW_PORT_STATUS_SPEED_10;
+ break;
+ default:
+ return;
+ }
+
+ if (state->duplex)
+ val |= AR9331_SW_PORT_STATUS_DUPLEX_MODE;
+
+ if (state->pause & MLO_PAUSE_TX)
+ val |= AR9331_SW_PORT_STATUS_TX_FLOW_EN;
+
+ if (state->pause & MLO_PAUSE_RX)
+ val |= AR9331_SW_PORT_STATUS_RX_FLOW_EN;
+
+ ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
+ AR9331_SW_PORT_STATUS_LINK_MASK, val);
+ if (ret)
+ dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+}
+
+static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface)
+{
+ struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+ struct regmap *regmap = priv->regmap;
+ int ret;
+
+ ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
+ AR9331_SW_PORT_STATUS_MAC_MASK, 0);
+ if (ret)
+ dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+}
+
+static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface,
+ struct phy_device *phydev)
+{
+ struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+ struct regmap *regmap = priv->regmap;
+ int ret;
+
+ ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
+ AR9331_SW_PORT_STATUS_MAC_MASK,
+ AR9331_SW_PORT_STATUS_MAC_MASK);
+ if (ret)
+ dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+}
+
+static const struct dsa_switch_ops ar9331_sw_ops = {
+ .get_tag_protocol = ar9331_sw_get_tag_protocol,
+ .setup = ar9331_sw_setup,
+ .port_enable = ar9331_sw_port_enable,
+ .port_disable = ar9331_sw_port_disable,
+ .phylink_validate = ar9331_sw_phylink_validate,
+ .phylink_mac_config = ar9331_sw_phylink_mac_config,
+ .phylink_mac_link_down = ar9331_sw_phylink_mac_link_down,
+ .phylink_mac_link_up = ar9331_sw_phylink_mac_link_up,
+};
+
+static irqreturn_t ar9331_sw_irq(int irq, void *data)
+{
+ struct ar9331_sw_priv *priv = data;
+ struct regmap *regmap = priv->regmap;
+ u32 stat;
+ int ret;
+
+ ret = regmap_read(regmap, AR9331_SW_REG_GINT, &stat);
+ if (ret) {
+ dev_err(priv->dev, "can't read interrupt status\n");
+ return IRQ_NONE;
+ }
+
+ if (!stat)
+ return IRQ_NONE;
+
+ if (stat & AR9331_SW_GINT_PHY_INT) {
+ int child_irq;
+
+ child_irq = irq_find_mapping(priv->irqdomain, 0);
+ handle_nested_irq(child_irq);
+ }
+
+ ret = regmap_write(regmap, AR9331_SW_REG_GINT, stat);
+ if (ret) {
+ dev_err(priv->dev, "can't write interrupt status\n");
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void ar9331_sw_mask_irq(struct irq_data *d)
+{
+ struct ar9331_sw_priv *priv = irq_data_get_irq_chip_data(d);
+ struct regmap *regmap = priv->regmap;
+ int ret;
+
+ ret = regmap_update_bits(regmap, AR9331_SW_REG_GINT_MASK,
+ AR9331_SW_GINT_PHY_INT, 0);
+ if (ret)
+ dev_err(priv->dev, "could not mask IRQ\n");
+}
+
+static void ar9331_sw_unmask_irq(struct irq_data *d)
+{
+ struct ar9331_sw_priv *priv = irq_data_get_irq_chip_data(d);
+ struct regmap *regmap = priv->regmap;
+ int ret;
+
+ ret = regmap_update_bits(regmap, AR9331_SW_REG_GINT_MASK,
+ AR9331_SW_GINT_PHY_INT,
+ AR9331_SW_GINT_PHY_INT);
+ if (ret)
+ dev_err(priv->dev, "could not unmask IRQ\n");
+}
+
+static struct irq_chip ar9331_sw_irq_chip = {
+ .name = AR9331_SW_NAME,
+ .irq_mask = ar9331_sw_mask_irq,
+ .irq_unmask = ar9331_sw_unmask_irq,
+};
+
+static int ar9331_sw_irq_map(struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ irq_set_chip_data(irq, domain->host_data);
+ irq_set_chip_and_handler(irq, &ar9331_sw_irq_chip, handle_simple_irq);
+ irq_set_nested_thread(irq, 1);
+ irq_set_noprobe(irq);
+
+ return 0;
+}
+
+static void ar9331_sw_irq_unmap(struct irq_domain *d, unsigned int irq)
+{
+ irq_set_nested_thread(irq, 0);
+ irq_set_chip_and_handler(irq, NULL, NULL);
+ irq_set_chip_data(irq, NULL);
+}
+
+static const struct irq_domain_ops ar9331_sw_irqdomain_ops = {
+ .map = ar9331_sw_irq_map,
+ .unmap = ar9331_sw_irq_unmap,
+ .xlate = irq_domain_xlate_onecell,
+};
+
+static int ar9331_sw_irq_init(struct ar9331_sw_priv *priv)
+{
+ struct device_node *np = priv->dev->of_node;
+ struct device *dev = priv->dev;
+ int ret, irq;
+
+ irq = of_irq_get(np, 0);
+ if (irq <= 0) {
+ dev_err(dev, "failed to get parent IRQ\n");
+ return irq ? irq : -EINVAL;
+ }
+
+ ret = devm_request_threaded_irq(dev, irq, NULL, ar9331_sw_irq,
+ IRQF_ONESHOT, AR9331_SW_NAME, priv);
+ if (ret) {
+ dev_err(dev, "unable to request irq: %d\n", ret);
+ return ret;
+ }
+
+ priv->irqdomain = irq_domain_add_linear(np, 1, &ar9331_sw_irqdomain_ops,
+ priv);
+ if (!priv->irqdomain) {
+ dev_err(dev, "failed to create IRQ domain\n");
+ return -EINVAL;
+ }
+
+ irq_set_parent(irq_create_mapping(priv->irqdomain, 0), irq);
+
+ return 0;
+}
+
+static int __ar9331_mdio_write(struct mii_bus *sbus, u8 mode, u16 reg, u16 val)
+{
+ u8 r, p;
+
+ p = FIELD_PREP(AR9331_SW_MDIO_PHY_MODE_M, mode) |
+ FIELD_GET(AR9331_SW_LOW_ADDR_PHY, reg);
+ r = FIELD_GET(AR9331_SW_LOW_ADDR_REG, reg);
+
+ return sbus->write(sbus, p, r, val);
+}
+
+static int __ar9331_mdio_read(struct mii_bus *sbus, u16 reg)
+{
+ u8 r, p;
+
+ p = FIELD_PREP(AR9331_SW_MDIO_PHY_MODE_M, AR9331_SW_MDIO_PHY_MODE_REG) |
+ FIELD_GET(AR9331_SW_LOW_ADDR_PHY, reg);
+ r = FIELD_GET(AR9331_SW_LOW_ADDR_REG, reg);
+
+ return sbus->read(sbus, p, r);
+}
+
+static int ar9331_mdio_read(void *ctx, const void *reg_buf, size_t reg_len,
+ void *val_buf, size_t val_len)
+{
+ struct ar9331_sw_priv *priv = ctx;
+ struct mii_bus *sbus = priv->sbus;
+ u32 reg = *(u32 *)reg_buf;
+ int ret;
+
+ if (reg == AR9331_SW_REG_PAGE) {
+ /* We cannot read the page selector register from hardware and
+ * we cache its value in regmap. Return all bits set here,
+ * that regmap will always write the page on first use.
+ */
+ *(u32 *)val_buf = GENMASK(9, 0);
+ return 0;
+ }
+
+ ret = __ar9331_mdio_read(sbus, reg);
+ if (ret < 0)
+ goto error;
+
+ *(u32 *)val_buf = ret;
+ ret = __ar9331_mdio_read(sbus, reg + 2);
+ if (ret < 0)
+ goto error;
+
+ *(u32 *)val_buf |= ret << 16;
+
+ return 0;
+error:
+ dev_err_ratelimited(&sbus->dev, "Bus error. Failed to read register.\n");
+ return ret;
+}
+
+static int ar9331_mdio_write(void *ctx, u32 reg, u32 val)
+{
+ struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ctx;
+ struct mii_bus *sbus = priv->sbus;
+ int ret;
+
+ if (reg == AR9331_SW_REG_PAGE) {
+ ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_PAGE,
+ 0, val);
+ if (ret < 0)
+ goto error;
+
+ return 0;
+ }
+
+ ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val);
+ if (ret < 0)
+ goto error;
+
+ ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2,
+ val >> 16);
+ if (ret < 0)
+ goto error;
+
+ return 0;
+error:
+ dev_err_ratelimited(&sbus->dev, "Bus error. Failed to write register.\n");
+ return ret;
+}
+
+static int ar9331_sw_bus_write(void *context, const void *data, size_t count)
+{
+ u32 reg = *(u32 *)data;
+ u32 val = *((u32 *)data + 1);
+
+ return ar9331_mdio_write(context, reg, val);
+}
+
+static const struct regmap_range ar9331_valid_regs[] = {
+ regmap_reg_range(0x0, 0x0),
+ regmap_reg_range(0x10, 0x14),
+ regmap_reg_range(0x20, 0x24),
+ regmap_reg_range(0x2c, 0x30),
+ regmap_reg_range(0x40, 0x44),
+ regmap_reg_range(0x50, 0x78),
+ regmap_reg_range(0x80, 0x98),
+
+ regmap_reg_range(0x100, 0x120),
+ regmap_reg_range(0x200, 0x220),
+ regmap_reg_range(0x300, 0x320),
+ regmap_reg_range(0x400, 0x420),
+ regmap_reg_range(0x500, 0x520),
+ regmap_reg_range(0x600, 0x620),
+
+ regmap_reg_range(0x20000, 0x200a4),
+ regmap_reg_range(0x20100, 0x201a4),
+ regmap_reg_range(0x20200, 0x202a4),
+ regmap_reg_range(0x20300, 0x203a4),
+ regmap_reg_range(0x20400, 0x204a4),
+ regmap_reg_range(0x20500, 0x205a4),
+
+ /* dummy page selector reg */
+ regmap_reg_range(AR9331_SW_REG_PAGE, AR9331_SW_REG_PAGE),
+};
+
+static const struct regmap_range ar9331_nonvolatile_regs[] = {
+ regmap_reg_range(AR9331_SW_REG_PAGE, AR9331_SW_REG_PAGE),
+};
+
+static const struct regmap_range_cfg ar9331_regmap_range[] = {
+ {
+ .selector_reg = AR9331_SW_REG_PAGE,
+ .selector_mask = GENMASK(9, 0),
+ .selector_shift = 0,
+
+ .window_start = 0,
+ .window_len = 512,
+
+ .range_min = 0,
+ .range_max = AR9331_SW_REG_PAGE - 4,
+ },
+};
+
+static const struct regmap_access_table ar9331_register_set = {
+ .yes_ranges = ar9331_valid_regs,
+ .n_yes_ranges = ARRAY_SIZE(ar9331_valid_regs),
+};
+
+static const struct regmap_access_table ar9331_volatile_set = {
+ .no_ranges = ar9331_nonvolatile_regs,
+ .n_no_ranges = ARRAY_SIZE(ar9331_nonvolatile_regs),
+};
+
+static const struct regmap_config ar9331_mdio_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register = AR9331_SW_REG_PAGE,
+
+ .ranges = ar9331_regmap_range,
+ .num_ranges = ARRAY_SIZE(ar9331_regmap_range),
+
+ .volatile_table = &ar9331_volatile_set,
+ .wr_table = &ar9331_register_set,
+ .rd_table = &ar9331_register_set,
+
+ .cache_type = REGCACHE_RBTREE,
+};
+
+static struct regmap_bus ar9331_sw_bus = {
+ .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+ .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+ .read = ar9331_mdio_read,
+ .write = ar9331_sw_bus_write,
+ .max_raw_read = 4,
+ .max_raw_write = 4,
+};
+
+static int ar9331_sw_probe(struct mdio_device *mdiodev)
+{
+ struct ar9331_sw_priv *priv;
+ int ret;
+
+ /* allocate the private data struct so that we can probe the switches
+ * ID register
+ */
+ priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->regmap = devm_regmap_init(&mdiodev->dev, &ar9331_sw_bus, priv,
+ &ar9331_mdio_regmap_config);
+ if (IS_ERR(priv->regmap)) {
+ ret = PTR_ERR(priv->regmap);
+ dev_err(&mdiodev->dev, "regmap init failed: %d\n", ret);
+ return ret;
+ }
+
+ priv->sw_reset = devm_reset_control_get(&mdiodev->dev, "switch");
+ if (IS_ERR(priv->sw_reset)) {
+ dev_err(&mdiodev->dev, "missing switch reset\n");
+ return PTR_ERR(priv->sw_reset);
+ }
+
+ priv->sbus = mdiodev->bus;
+ priv->dev = &mdiodev->dev;
+
+ ret = ar9331_sw_irq_init(priv);
+ if (ret)
+ return ret;
+
+ priv->ds = dsa_switch_alloc(&mdiodev->dev, AR9331_SW_PORTS);
+ if (!priv->ds)
+ return -ENOMEM;
+
+ priv->ds->priv = priv;
+ priv->ops = ar9331_sw_ops;
+ priv->ds->ops = &priv->ops;
+ dev_set_drvdata(&mdiodev->dev, priv);
+
+ return dsa_register_switch(priv->ds);
+}
+
+static void ar9331_sw_remove(struct mdio_device *mdiodev)
+{
+ struct ar9331_sw_priv *priv = dev_get_drvdata(&mdiodev->dev);
+
+ mdiobus_unregister(priv->mbus);
+ dsa_unregister_switch(priv->ds);
+
+ reset_control_assert(priv->sw_reset);
+}
+
+static const struct of_device_id ar9331_sw_of_match[] = {
+ { .compatible = "qca,ar9331-switch" },
+ { },
+};
+
+static struct mdio_driver ar9331_sw_mdio_driver = {
+ .probe = ar9331_sw_probe,
+ .remove = ar9331_sw_remove,
+ .mdiodrv.driver = {
+ .name = AR9331_SW_NAME,
+ .of_match_table = ar9331_sw_of_match,
+ },
+};
+
+mdio_module_driver(ar9331_sw_mdio_driver);
+
+MODULE_AUTHOR("Oleksij Rempel <[email protected]>");
+MODULE_DESCRIPTION("Driver for Atheros AR9331 switch");
+MODULE_LICENSE("GPL v2");
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 541fb514e31d..89a334e68d42 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -42,6 +42,7 @@ struct phylink_link_state;
#define DSA_TAG_PROTO_8021Q_VALUE 12
#define DSA_TAG_PROTO_SJA1105_VALUE 13
#define DSA_TAG_PROTO_KSZ8795_VALUE 14
+#define DSA_TAG_PROTO_AR9331_VALUE 15

enum dsa_tag_protocol {
DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
@@ -59,6 +60,7 @@ enum dsa_tag_protocol {
DSA_TAG_PROTO_8021Q = DSA_TAG_PROTO_8021Q_VALUE,
DSA_TAG_PROTO_SJA1105 = DSA_TAG_PROTO_SJA1105_VALUE,
DSA_TAG_PROTO_KSZ8795 = DSA_TAG_PROTO_KSZ8795_VALUE,
+ DSA_TAG_PROTO_AR9331 = DSA_TAG_PROTO_AR9331_VALUE,
};

struct packet_type;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 29e2bd5cc5af..6e015309a7fe 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -107,4 +107,10 @@ config NET_DSA_TAG_TRAILER
Say Y or M if you want to enable support for tagging frames at
with a trailed. e.g. Marvell 88E6060.

+config NET_DSA_TAG_AR9331
+ tristate "Tag driver for Atheros AR9331 SoC with build-in switch"
+ help
+ Say Y or M if you want to enable support for tagging frames for
+ the Atheros AR9331 SoC with build-in switch.
+
endif
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 2c6d286f0511..67caebf602be 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
obj-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o
obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
+obj-$(CONFIG_NET_DSA_TAG_AR9331) += tag_ar9331.o
diff --git a/net/dsa/tag_ar9331.c b/net/dsa/tag_ar9331.c
new file mode 100644
index 000000000000..b32a8d3d48b9
--- /dev/null
+++ b/net/dsa/tag_ar9331.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 Pengutronix, Oleksij Rempel <[email protected]>
+ */
+
+
+#include <linux/bitfield.h>
+#include <linux/etherdevice.h>
+
+#include "dsa_priv.h"
+
+#define AR9331_HDR_LEN 2
+#define AR9331_HDR_VERSION 1
+
+#define AR9331_HDR_VERSION_MASK GENMASK(15, 14)
+#define AR9331_HDR_PRIORITY_MASK GENMASK(13, 12)
+#define AR9331_HDR_TYPE_MASK GENMASK(10, 8)
+#define AR9331_HDR_BROADCAST BIT(7)
+#define AR9331_HDR_FROM_CPU BIT(6)
+/* AR9331_HDR_RESERVED - not used or may be version filed.
+ * According to the AR8216 doc it should 0b10. On AR9331 it is 0b11 on RX path
+ * and should be set to 0b11 to make it work.
+ */
+#define AR9331_HDR_RESERVED_MASK GENMASK(5, 4)
+#define AR9331_HDR_PORT_NUM_MASK GENMASK(3, 0)
+
+static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ struct dsa_port *dp = dsa_slave_to_port(dev);
+ __le16 *phdr;
+ u16 hdr;
+
+ if (skb_cow_head(skb, 0) < 0)
+ return NULL;
+
+ phdr = skb_push(skb, AR9331_HDR_LEN);
+
+ hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);
+ hdr |= AR9331_HDR_FROM_CPU | dp->index;
+ /* 0b10 for AR8216 and 0b11 for AR9331 */
+ hdr |= AR9331_HDR_RESERVED_MASK;
+
+ phdr[0] = cpu_to_le16(hdr);
+
+ return skb;
+}
+
+static struct sk_buff *ar9331_tag_rcv(struct sk_buff *skb,
+ struct net_device *ndev,
+ struct packet_type *pt)
+{
+ u8 ver, port;
+ u16 hdr;
+
+ if (unlikely(!pskb_may_pull(skb, AR9331_HDR_LEN)))
+ return NULL;
+
+ hdr = le16_to_cpu(*(__le16 *)skb_mac_header(skb));
+
+ ver = FIELD_GET(AR9331_HDR_VERSION_MASK, hdr);
+ if (unlikely(ver != AR9331_HDR_VERSION)) {
+ netdev_warn(ndev, "%s:%i wrong header version 0x%2x\n",
+ __func__, __LINE__, hdr);
+ return NULL;
+ }
+
+ if (unlikely(hdr & AR9331_HDR_FROM_CPU)) {
+ netdev_warn(ndev, "%s:%i packet should not be from cpu 0x%2x\n",
+ __func__, __LINE__, hdr);
+ return NULL;
+ }
+
+ skb_pull(skb, AR9331_HDR_LEN);
+ skb_set_mac_header(skb, -ETH_HLEN);
+
+ /* Get source port information */
+ port = FIELD_GET(AR9331_HDR_PORT_NUM_MASK, hdr);
+
+ skb->dev = dsa_master_find_slave(ndev, 0, port);
+ if (!skb->dev)
+ return NULL;
+
+ return skb;
+}
+
+static const struct dsa_device_ops ar9331_netdev_ops = {
+ .name = "ar9331",
+ .proto = DSA_TAG_PROTO_AR9331,
+ .xmit = ar9331_tag_xmit,
+ .rcv = ar9331_tag_rcv,
+ .overhead = AR9331_HDR_LEN,
+};
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_AR9331);
+module_dsa_tag_driver(ar9331_netdev_ops);
--
2.23.0

2019-10-14 06:17:56

by Oleksij Rempel

[permalink] [raw]
Subject: [PATCH v1 2/4] dt-bindings: net: dsa: qca,ar9331 switch documentation

Atheros AR9331 has built-in 5 port switch. The switch can be configured
to use all 5 or 4 ports. One of built-in PHYs can be used by first built-in
ethernet controller or to be used directly by the switch over second ethernet
controller.

Signed-off-by: Oleksij Rempel <[email protected]>
---
.../devicetree/bindings/net/dsa/ar9331.txt | 155 ++++++++++++++++++
1 file changed, 155 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/dsa/ar9331.txt

diff --git a/Documentation/devicetree/bindings/net/dsa/ar9331.txt b/Documentation/devicetree/bindings/net/dsa/ar9331.txt
new file mode 100644
index 000000000000..b0f95fd19584
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/ar9331.txt
@@ -0,0 +1,155 @@
+Atheros AR9331 built-in switch
+=============================
+
+It is a switch built-in to Atheros AR9331 WiSoC and addressable over internal
+MDIO bus. All PHYs are build-in as well.
+
+Required properties:
+
+ - compatible: should be: "qca,ar9331-switch"
+ - reg: Address on the MII bus for the switch.
+ - resets : Must contain an entry for each entry in reset-names.
+ - reset-names : Must include the following entries: "switch"
+ - interrupt-parent: Phandle to the parent interrupt controller
+ - interrupts: IRQ line for the switch
+ - interrupt-controller: Indicates the switch is itself an interrupt
+ controller. This is used for the PHY interrupts.
+ - #interrupt-cells: must be 1
+ - mdio: Container of PHY and devices on the switches MDIO bus.
+
+See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
+required and optional properties.
+Examples:
+
+eth0: ethernet@19000000 {
+ compatible = "qca,ar9330-eth";
+ reg = <0x19000000 0x200>;
+ interrupts = <4>;
+
+ resets = <&rst 9>, <&rst 22>;
+ reset-names = "mac", "mdio";
+ clocks = <&pll ATH79_CLK_AHB>, <&pll ATH79_CLK_AHB>;
+ clock-names = "eth", "mdio";
+
+ phy-mode = "mii";
+ phy-handle = <&phy_port4>;
+};
+
+eth1: ethernet@1a000000 {
+ compatible = "qca,ar9330-eth";
+ reg = <0x1a000000 0x200>;
+ interrupts = <5>;
+ resets = <&rst 13>, <&rst 23>;
+ reset-names = "mac", "mdio";
+ clocks = <&pll ATH79_CLK_AHB>, <&pll ATH79_CLK_AHB>;
+ clock-names = "eth", "mdio";
+
+ phy-mode = "gmii";
+ phy-handle = <&switch_port0>;
+
+ fixed-link {
+ speed = <1000>;
+ full-duplex;
+ };
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ switch10: switch@10 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ compatible = "qca,ar9331-switch";
+ reg = <16>;
+ resets = <&rst 8>;
+ reset-names = "switch";
+
+ interrupt-parent = <&miscintc>;
+ interrupts = <12>;
+
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ switch_port0: port@0 {
+ reg = <0>;
+ label = "cpu";
+ ethernet = <&eth1>;
+
+ phy-mode = "gmii";
+
+ fixed-link {
+ speed = <1000>;
+ full-duplex;
+ };
+ };
+
+ switch_port1: port@1 {
+ reg = <1>;
+ phy-handle = <&phy_port0>;
+ phy-mode = "internal";
+ };
+
+ switch_port2: port@2 {
+ reg = <2>;
+ phy-handle = <&phy_port1>;
+ phy-mode = "internal";
+ };
+
+ switch_port3: port@3 {
+ reg = <3>;
+ phy-handle = <&phy_port2>;
+ phy-mode = "internal";
+ };
+
+ switch_port4: port@4 {
+ reg = <4>;
+ phy-handle = <&phy_port3>;
+ phy-mode = "internal";
+ };
+
+ switch_port5: port@5 {
+ reg = <5>;
+ phy-handle = <&phy_port4>;
+ phy-mode = "internal";
+ };
+ };
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ interrupt-parent = <&switch10>;
+
+ phy_port0: phy@0 {
+ reg = <0>;
+ interrupts = <0>;
+ };
+
+ phy_port1: phy@1 {
+ reg = <1>;
+ interrupts = <0>;
+ };
+
+ phy_port2: phy@2 {
+ reg = <2>;
+ interrupts = <0>;
+ };
+
+ phy_port3: phy@3 {
+ reg = <3>;
+ interrupts = <0>;
+ };
+
+ phy_port4: phy@4 {
+ reg = <4>;
+ interrupts = <0>;
+ };
+ };
+ };
+ };
+};
--
2.23.0

2019-10-16 15:28:12

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v1 2/4] dt-bindings: net: dsa: qca,ar9331 switch documentation

On Mon, Oct 14, 2019 at 08:15:47AM +0200, Oleksij Rempel wrote:
> Atheros AR9331 has built-in 5 port switch. The switch can be configured
> to use all 5 or 4 ports. One of built-in PHYs can be used by first built-in
> ethernet controller or to be used directly by the switch over second ethernet
> controller.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/ar9331.txt | 155 ++++++++++++++++++
> 1 file changed, 155 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/dsa/ar9331.txt
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/ar9331.txt b/Documentation/devicetree/bindings/net/dsa/ar9331.txt
> new file mode 100644
> index 000000000000..b0f95fd19584
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/ar9331.txt
> @@ -0,0 +1,155 @@
> +Atheros AR9331 built-in switch
> +=============================
> +
> +It is a switch built-in to Atheros AR9331 WiSoC and addressable over internal
> +MDIO bus. All PHYs are build-in as well.
> +
> +Required properties:
> +
> + - compatible: should be: "qca,ar9331-switch"
> + - reg: Address on the MII bus for the switch.
> + - resets : Must contain an entry for each entry in reset-names.
> + - reset-names : Must include the following entries: "switch"
> + - interrupt-parent: Phandle to the parent interrupt controller
> + - interrupts: IRQ line for the switch
> + - interrupt-controller: Indicates the switch is itself an interrupt
> + controller. This is used for the PHY interrupts.
> + - #interrupt-cells: must be 1
> + - mdio: Container of PHY and devices on the switches MDIO bus.
> +
> +See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
> +required and optional properties.
> +Examples:
> +
> +eth0: ethernet@19000000 {
> + compatible = "qca,ar9330-eth";
> + reg = <0x19000000 0x200>;
> + interrupts = <4>;
> +
> + resets = <&rst 9>, <&rst 22>;
> + reset-names = "mac", "mdio";
> + clocks = <&pll ATH79_CLK_AHB>, <&pll ATH79_CLK_AHB>;
> + clock-names = "eth", "mdio";
> +
> + phy-mode = "mii";
> + phy-handle = <&phy_port4>;

This does not seem like a valid example. If phy_port4 is listed here,
i would expect switch_port 5 to be totally missing?

> +};
> +
> +eth1: ethernet@1a000000 {
> + compatible = "qca,ar9330-eth";
> + reg = <0x1a000000 0x200>;
> + interrupts = <5>;
> + resets = <&rst 13>, <&rst 23>;
> + reset-names = "mac", "mdio";
> + clocks = <&pll ATH79_CLK_AHB>, <&pll ATH79_CLK_AHB>;
> + clock-names = "eth", "mdio";
> +
> + phy-mode = "gmii";
> + phy-handle = <&switch_port0>;
> +
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };

You also cannot have both a fixed-link and a phy-handle.

> +
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + switch10: switch@10 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + compatible = "qca,ar9331-switch";
> + reg = <16>;

Maybe don't mix up hex and decimal? switch16: switch@16.

Andrew

2019-10-16 15:37:58

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH v1 2/4] dt-bindings: net: dsa: qca,ar9331 switch documentation

On Wed, Oct 16, 2019 at 02:21:52PM +0200, Andrew Lunn wrote:
> On Mon, Oct 14, 2019 at 08:15:47AM +0200, Oleksij Rempel wrote:
> > Atheros AR9331 has built-in 5 port switch. The switch can be configured
> > to use all 5 or 4 ports. One of built-in PHYs can be used by first built-in
> > ethernet controller or to be used directly by the switch over second ethernet
> > controller.
> >
> > Signed-off-by: Oleksij Rempel <[email protected]>
> > ---
> > .../devicetree/bindings/net/dsa/ar9331.txt | 155 ++++++++++++++++++
> > 1 file changed, 155 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/net/dsa/ar9331.txt
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/ar9331.txt b/Documentation/devicetree/bindings/net/dsa/ar9331.txt
> > new file mode 100644
> > index 000000000000..b0f95fd19584
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/dsa/ar9331.txt
> > @@ -0,0 +1,155 @@
> > +Atheros AR9331 built-in switch
> > +=============================
> > +
> > +It is a switch built-in to Atheros AR9331 WiSoC and addressable over internal
> > +MDIO bus. All PHYs are build-in as well.
> > +
> > +Required properties:
> > +
> > + - compatible: should be: "qca,ar9331-switch"
> > + - reg: Address on the MII bus for the switch.
> > + - resets : Must contain an entry for each entry in reset-names.
> > + - reset-names : Must include the following entries: "switch"
> > + - interrupt-parent: Phandle to the parent interrupt controller
> > + - interrupts: IRQ line for the switch
> > + - interrupt-controller: Indicates the switch is itself an interrupt
> > + controller. This is used for the PHY interrupts.
> > + - #interrupt-cells: must be 1
> > + - mdio: Container of PHY and devices on the switches MDIO bus.
> > +
> > +See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
> > +required and optional properties.
> > +Examples:
> > +
> > +eth0: ethernet@19000000 {
> > + compatible = "qca,ar9330-eth";
> > + reg = <0x19000000 0x200>;
> > + interrupts = <4>;
> > +
> > + resets = <&rst 9>, <&rst 22>;
> > + reset-names = "mac", "mdio";
> > + clocks = <&pll ATH79_CLK_AHB>, <&pll ATH79_CLK_AHB>;
> > + clock-names = "eth", "mdio";
> > +
> > + phy-mode = "mii";
> > + phy-handle = <&phy_port4>;
>
> This does not seem like a valid example. If phy_port4 is listed here,
> i would expect switch_port 5 to be totally missing?

hm... right.
phy4 can be used with switch_port 5 or eth0. Should i remove completely
switch_port 5 node or it is enough to "disable" it.

> > +};
> > +
> > +eth1: ethernet@1a000000 {
> > + compatible = "qca,ar9330-eth";
> > + reg = <0x1a000000 0x200>;
> > + interrupts = <5>;
> > + resets = <&rst 13>, <&rst 23>;
> > + reset-names = "mac", "mdio";
> > + clocks = <&pll ATH79_CLK_AHB>, <&pll ATH79_CLK_AHB>;
> > + clock-names = "eth", "mdio";
> > +
> > + phy-mode = "gmii";
> > + phy-handle = <&switch_port0>;
> > +
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + };
>
> You also cannot have both a fixed-link and a phy-handle.

ok.

>
> > +
> > + mdio {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + switch10: switch@10 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + compatible = "qca,ar9331-switch";
> > + reg = <16>;
>
> Maybe don't mix up hex and decimal? switch16: switch@16.

ok. will fix it. What is actually proper way to set the reg of switch?
This switch is responding on range of phy addresses: any of two high bits of 5
bit phy address.

Regards,
Oleksij

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2019-10-16 16:01:35

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v1 4/4] net: dsa: add support for Atheros AR9331 build-in switch

> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -0,0 +1,822 @@
> +// SPDX-License-Identifier: GPL-2.0-only

I think C files should use /* */, and header files //, for SPDX.

> +// Copyright (c) 2019 Pengutronix, Oleksij Rempel <[email protected]>
> +
> +#include <linux/bitfield.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_mdio.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <net/dsa.h>
> +
> +#define AR9331_SW_NAME "ar9331_switch"
> +#define AR9331_SW_PORTS 6
> +
> +/* dummy reg to change page */
> +#define AR9331_SW_REG_PAGE BIT(18)
> +
> +/* Global Interrupt */
> +#define AR9331_SW_REG_GINT 0x10
> +#define AR9331_SW_REG_GINT_MASK 0x14
> +#define AR9331_SW_GINT_PHY_INT BIT(2)
> +
> +#define AR9331_SW_REG_FLOOD_MASK 0x2c
> +#define AR9331_SW_FLOOD_MASK_BROAD_TO_CPU BIT(26)
> +
> +#define AR9331_SW_REG_GLOBAL_CTRL 0x30
> +#define AR9331_SW_GLOBAL_CTRL_MFS_M GENMASK(13, 0)
> +
> +#define AR9331_SW_REG_MDIO_CTRL 0x98
> +#define AR9331_SW_MDIO_CTRL_BUSY BIT(31)
> +#define AR9331_SW_MDIO_CTRL_MASTER_EN BIT(30)
> +#define AR9331_SW_MDIO_CTRL_CMD_READ BIT(27)
> +#define AR9331_SW_MDIO_CTRL_PHY_ADDR_M GENMASK(25, 21)
> +#define AR9331_SW_MDIO_CTRL_REG_ADDR_M GENMASK(20, 16)
> +#define AR9331_SW_MDIO_CTRL_DATA_M GENMASK(16, 0)
> +
> +#define AR9331_SW_REG_PORT_STATUS(_port) (0x100 + (_port) * 0x100)
> +
> +/* FLOW_LINK_EN - enable mac flow control config auto-neg with phy.
> + * If not set, mac can be config by software.
> + */
> +#define AR9331_SW_PORT_STATUS_FLOW_LINK_EN BIT(12)
> +
> +/* LINK_EN - If set, MAC is configured from PHY link status.
> + * If not set, MAC should be configured by software.
> + */
> +#define AR9331_SW_PORT_STATUS_LINK_EN BIT(9)
> +#define AR9331_SW_PORT_STATUS_DUPLEX_MODE BIT(6)
> +#define AR9331_SW_PORT_STATUS_RX_FLOW_EN BIT(5)
> +#define AR9331_SW_PORT_STATUS_TX_FLOW_EN BIT(4)
> +#define AR9331_SW_PORT_STATUS_RXMAC BIT(3)
> +#define AR9331_SW_PORT_STATUS_TXMAC BIT(2)
> +#define AR9331_SW_PORT_STATUS_SPEED_M GENMASK(1, 0)
> +#define AR9331_SW_PORT_STATUS_SPEED_1000 2
> +#define AR9331_SW_PORT_STATUS_SPEED_100 1
> +#define AR9331_SW_PORT_STATUS_SPEED_10 0
> +
> +#define AR9331_SW_PORT_STATUS_MAC_MASK \
> + (AR9331_SW_PORT_STATUS_TXMAC | AR9331_SW_PORT_STATUS_RXMAC)
> +
> +#define AR9331_SW_PORT_STATUS_LINK_MASK \
> + (AR9331_SW_PORT_STATUS_LINK_EN | AR9331_SW_PORT_STATUS_FLOW_LINK_EN | \
> + AR9331_SW_PORT_STATUS_DUPLEX_MODE | \
> + AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
> + AR9331_SW_PORT_STATUS_SPEED_M)
> +
> +/* Phy bypass mode
> + * ------------------------------------------------------------------------
> + * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> + *
> + * real | start | OP | PhyAddr | Reg Addr | TA |
> + * atheros| start | OP | 2'b00 |PhyAdd[2:0]| Reg Addr[4:0] | TA |
> + *
> + *
> + * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
> + * real | Data |
> + * atheros| Data |
> + *
> + * ------------------------------------------------------------------------
> + * Page address mode
> + * ------------------------------------------------------------------------
> + * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> + * real | start | OP | PhyAddr | Reg Addr | TA |
> + * atheros| start | OP | 2'b11 | 8'b0 | TA |
> + *
> + * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
> + * real | Data |
> + * atheros| | Page [9:0] |
> + */
> +/* In case of Page Address mode, Bit[18:9] of 32 bit register address should be
> + * written to bits[9:0] of mdio data register.
> + */
> +#define AR9331_SW_ADDR_PAGE GENMASK(18, 9)
> +
> +/* ------------------------------------------------------------------------
> + * Normal register access mode
> + * ------------------------------------------------------------------------
> + * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> + * real | start | OP | PhyAddr | Reg Addr | TA |
> + * atheros| start | OP | 2'b10 | low_addr[7:0] | TA |
> + *
> + * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
> + * real | Data |
> + * atheros| Data |
> + * ------------------------------------------------------------------------
> + */
> +#define AR9331_SW_LOW_ADDR_PHY GENMASK(8, 6)
> +#define AR9331_SW_LOW_ADDR_REG GENMASK(5, 1)
> +
> +#define AR9331_SW_MDIO_PHY_MODE_M GENMASK(4, 3)
> +#define AR9331_SW_MDIO_PHY_MODE_PAGE 3
> +#define AR9331_SW_MDIO_PHY_MODE_REG 2
> +#define AR9331_SW_MDIO_PHY_MODE_BYPASS 0
> +#define AR9331_SW_MDIO_PHY_ADDR_M GENMASK(2, 0)
> +
> +/* Empirical determined values */
> +#define AR9331_SW_MDIO_POLL_SLEEP_US 1
> +#define AR9331_SW_MDIO_POLL_TIMEOUT_US 20
> +
> +struct ar9331_sw_priv {
> + struct device *dev;
> + struct dsa_switch *ds;
> + struct dsa_switch_ops ops;
> + struct irq_domain *irqdomain;
> + struct mii_bus *mbus; /* mdio master */
> + struct mii_bus *sbus; /* mdio slave */
> + struct regmap *regmap;
> + struct reset_control *sw_reset;
> +};
> +
> +/* Warning: switch reset will reset last AR9331_SW_MDIO_PHY_MODE_PAGE request
> + * If some kind of optimization is used, the request should be repeated.
> + */
> +static int ar9331_sw_reset(struct ar9331_sw_priv *priv)
> +{
> + int ret;
> +
> + ret = reset_control_assert(priv->sw_reset);
> + if (ret)
> + goto error;
> +
> + /* AR9331 doc do not provide any information about proper reset
> + * sequence. The AR8136 (the closes switch to the AR9331) doc says:
> + * reset duration should be greater than 10ms. So, let's use this value
> + * for now.
> + */
> + usleep_range(10000, 15000);
> + ret = reset_control_deassert(priv->sw_reset);
> + if (ret)
> + goto error;

Any comments in the documentation about needing to wait for the reset
to complete?


> +
> + return 0;
> +error:
> + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> + return ret;
> +}
> +
> +static int ar9331_sw_mbus_write(struct mii_bus *mbus, int port, int regnum,
> + u16 data)
> +{
> + struct ar9331_sw_priv *priv = mbus->priv;
> + struct regmap *regmap = priv->regmap;
> + u32 val;
> + int ret;
> +
> + ret = regmap_write(regmap, AR9331_SW_REG_MDIO_CTRL,
> + AR9331_SW_MDIO_CTRL_BUSY |
> + AR9331_SW_MDIO_CTRL_MASTER_EN |
> + FIELD_PREP(AR9331_SW_MDIO_CTRL_PHY_ADDR_M, port) |
> + FIELD_PREP(AR9331_SW_MDIO_CTRL_REG_ADDR_M, regnum) |
> + FIELD_PREP(AR9331_SW_MDIO_CTRL_DATA_M, data));
> + if (ret)
> + goto error;
> +
> + ret = regmap_read_poll_timeout(regmap, AR9331_SW_REG_MDIO_CTRL, val,
> + !(val & AR9331_SW_MDIO_CTRL_BUSY),
> + AR9331_SW_MDIO_POLL_SLEEP_US,
> + AR9331_SW_MDIO_POLL_TIMEOUT_US);
> + if (ret)
> + goto error;
> +
> + return 0;
> +error:
> + dev_err_ratelimited(priv->dev, "PHY write error: %i\n", ret);
> + return ret;
> +}
> +
> +static int ar9331_sw_mbus_read(struct mii_bus *mbus, int port, int regnum)
> +{
> + struct ar9331_sw_priv *priv = mbus->priv;
> + struct regmap *regmap = priv->regmap;
> + u32 val;
> + int ret;
> +
> + ret = regmap_write(regmap, AR9331_SW_REG_MDIO_CTRL,
> + AR9331_SW_MDIO_CTRL_BUSY |
> + AR9331_SW_MDIO_CTRL_MASTER_EN |
> + AR9331_SW_MDIO_CTRL_CMD_READ |
> + FIELD_PREP(AR9331_SW_MDIO_CTRL_PHY_ADDR_M, port) |
> + FIELD_PREP(AR9331_SW_MDIO_CTRL_REG_ADDR_M, regnum));
> + if (ret)
> + goto error;
> +
> + ret = regmap_read_poll_timeout(regmap, AR9331_SW_REG_MDIO_CTRL, val,
> + !(val & AR9331_SW_MDIO_CTRL_BUSY),
> + AR9331_SW_MDIO_POLL_SLEEP_US,
> + AR9331_SW_MDIO_POLL_TIMEOUT_US);
> + if (ret)
> + goto error;
> +
> + ret = regmap_read(regmap, AR9331_SW_REG_MDIO_CTRL, &val);
> + if (ret)
> + goto error;
> +
> + return FIELD_GET(AR9331_SW_MDIO_CTRL_DATA_M, val);
> +
> +error:
> + dev_err_ratelimited(priv->dev, "PHY read error: %i\n", ret);
> + return ret;
> +}
> +
> +static int ar9331_sw_mbus_init(struct ar9331_sw_priv *priv)
> +{
> + struct device *dev = priv->dev;
> + static struct mii_bus *mbus;
> + struct device_node *np, *mnp;
> + int ret;
> +
> + np = dev->of_node;
> +
> + mbus = devm_mdiobus_alloc(dev);
> + if (!mbus)
> + return -ENOMEM;
> +
> + mbus->name = np->full_name;
> + snprintf(mbus->id, MII_BUS_ID_SIZE, "%pOF", np);
> +
> + mbus->read = ar9331_sw_mbus_read;
> + mbus->write = ar9331_sw_mbus_write;
> + mbus->priv = priv;
> + mbus->parent = dev;
> +
> + mnp = of_get_child_by_name(np, "mdio");

You should check if mnp is NULL. You want it to mandatory. The current
code will pass NULL to of_mdiobus_register(), which is legal, and it
will look one level higher for the PHYs.


> + ret = of_mdiobus_register(mbus, mnp);
> + of_node_put(mnp);
> + if (ret)
> + return ret;
> +
> + priv->mbus = mbus;
> +
> + return 0;
> +}
> +
> +static int ar9331_sw_setup(struct dsa_switch *ds)
> +{
> + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> + struct regmap *regmap = priv->regmap;
> + int ret;
> +
> + ret = ar9331_sw_reset(priv);
> + if (ret)
> + return ret;
> +
> + /* Reset will set proper defaults. CPU - Port0 will be enabled and
> + * configured. All other ports (ports 1 - 5) are disabled
> + */

Nice, some hardware engineer thought about that.

> + ret = ar9331_sw_mbus_init(priv);
> + if (ret)
> + return ret;
> +
> + /* Do not drop broadcast frames */
> + ret = regmap_write_bits(regmap, AR9331_SW_REG_FLOOD_MASK,
> + AR9331_SW_FLOOD_MASK_BROAD_TO_CPU,
> + AR9331_SW_FLOOD_MASK_BROAD_TO_CPU);
> + if (ret)
> + goto error;
> +
> + /* Sync max frame size with value used in
> + * drivers/net/ethernet/atheros/ag71xx.c for ar9330 SoC.
> + * TODO: In both drivers this value seems to be not real maximal size
> + * The switch is able to configure 0x3fff and ethernet controller
> + * 0xffff. Are there any better way to sync this values?
> + */
> + ret = regmap_write_bits(regmap, AR9331_SW_REG_GLOBAL_CTRL,
> + AR9331_SW_GLOBAL_CTRL_MFS_M,
> + FIELD_PREP(AR9331_SW_GLOBAL_CTRL_MFS_M, 1540));
> + if (ret)
> + goto error;

Jumbo is not so easy. I would avoid the plain number 1540. There
should be a #define. Also, you might want to allow space for a VLAN
header? Does enabbling jumbo have a performance impact? If not, you
can configure the switch to its maximum size.

> +
> + return 0;
> +error:
> + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> + return ret;
> +}
> +
> +static int ar9331_sw_port_enable(struct dsa_switch *ds, int port,
> + struct phy_device *phy)
> +{
> + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> + struct regmap *regmap = priv->regmap;
> + int ret;
> +
> + /* nothing to enable. Just set link to initial state */
> + ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
> + if (ret)
> + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +
> + return ret;
> +}
> +
> +static void ar9331_sw_port_disable(struct dsa_switch *ds, int port)
> +{
> + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> + struct regmap *regmap = priv->regmap;
> + int ret;
> +
> + ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
> + if (ret)
> + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +}

port_enable() and port_disable() look the same?

> +
> +static enum dsa_tag_protocol ar9331_sw_get_tag_protocol(struct dsa_switch *ds,
> + int port)
> +{
> + return DSA_TAG_PROTO_AR9331;
> +}
> +
> +static void ar9331_sw_phylink_validate(struct dsa_switch *ds, int port,
> + unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> + switch (port) {
> + case 0:
> + if (state->interface != PHY_INTERFACE_MODE_GMII)
> + goto unsupported;
> +
> + phylink_set(mask, 1000baseT_Full);
> + phylink_set(mask, 1000baseT_Half);
> + break;
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + case 5:
> + if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> + goto unsupported;
> + break;
> + default:
> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> + dev_err(ds->dev, "Unsupported port: %i\n", port);
> + return;
> + }
> +
> + phylink_set_port_modes(mask);
> + phylink_set(mask, Pause);
> + phylink_set(mask, Asym_Pause);
> +
> + phylink_set(mask, 10baseT_Half);
> + phylink_set(mask, 10baseT_Full);
> + phylink_set(mask, 100baseT_Half);
> + phylink_set(mask, 100baseT_Full);

So the CPU port is 1G capable. All the other ports are only Fast Ethernet?

> +
> + bitmap_and(supported, supported, mask,
> + __ETHTOOL_LINK_MODE_MASK_NBITS);
> + bitmap_and(state->advertising, state->advertising, mask,
> + __ETHTOOL_LINK_MODE_MASK_NBITS);
> +
> + return;
> +
> +unsupported:
> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> + dev_err(ds->dev, "Unsupported interface: %d, port: %d\n",
> + state->interface, port);
> +}
> +
> +static void ar9331_sw_phylink_mac_config(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> + struct regmap *regmap = priv->regmap;
> + int ret;
> + u32 val;
> +
> + switch (state->speed) {
> + case SPEED_1000:
> + val = AR9331_SW_PORT_STATUS_SPEED_1000;
> + break;
> + case SPEED_100:
> + val = AR9331_SW_PORT_STATUS_SPEED_100;
> + break;
> + case SPEED_10:
> + val = AR9331_SW_PORT_STATUS_SPEED_10;
> + break;
> + default:
> + return;
> + }
> +
> + if (state->duplex)
> + val |= AR9331_SW_PORT_STATUS_DUPLEX_MODE;
> +
> + if (state->pause & MLO_PAUSE_TX)
> + val |= AR9331_SW_PORT_STATUS_TX_FLOW_EN;
> +
> + if (state->pause & MLO_PAUSE_RX)
> + val |= AR9331_SW_PORT_STATUS_RX_FLOW_EN;
> +
> + ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
> + AR9331_SW_PORT_STATUS_LINK_MASK, val);
> + if (ret)
> + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +}
> +
> +static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface)
> +{
> + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> + struct regmap *regmap = priv->regmap;
> + int ret;
> +
> + ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
> + AR9331_SW_PORT_STATUS_MAC_MASK, 0);
> + if (ret)
> + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +}
> +
> +static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev)
> +{
> + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> + struct regmap *regmap = priv->regmap;
> + int ret;
> +
> + ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
> + AR9331_SW_PORT_STATUS_MAC_MASK,
> + AR9331_SW_PORT_STATUS_MAC_MASK);
> + if (ret)
> + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +}
> +
> +static const struct dsa_switch_ops ar9331_sw_ops = {
> + .get_tag_protocol = ar9331_sw_get_tag_protocol,
> + .setup = ar9331_sw_setup,
> + .port_enable = ar9331_sw_port_enable,
> + .port_disable = ar9331_sw_port_disable,
> + .phylink_validate = ar9331_sw_phylink_validate,
> + .phylink_mac_config = ar9331_sw_phylink_mac_config,
> + .phylink_mac_link_down = ar9331_sw_phylink_mac_link_down,
> + .phylink_mac_link_up = ar9331_sw_phylink_mac_link_up,
> +};
> +
> +static irqreturn_t ar9331_sw_irq(int irq, void *data)
> +{
> + struct ar9331_sw_priv *priv = data;
> + struct regmap *regmap = priv->regmap;
> + u32 stat;
> + int ret;
> +
> + ret = regmap_read(regmap, AR9331_SW_REG_GINT, &stat);
> + if (ret) {
> + dev_err(priv->dev, "can't read interrupt status\n");
> + return IRQ_NONE;
> + }
> +
> + if (!stat)
> + return IRQ_NONE;
> +
> + if (stat & AR9331_SW_GINT_PHY_INT) {
> + int child_irq;
> +
> + child_irq = irq_find_mapping(priv->irqdomain, 0);
> + handle_nested_irq(child_irq);
> + }
> +
> + ret = regmap_write(regmap, AR9331_SW_REG_GINT, stat);
> + if (ret) {
> + dev_err(priv->dev, "can't write interrupt status\n");
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void ar9331_sw_mask_irq(struct irq_data *d)
> +{
> + struct ar9331_sw_priv *priv = irq_data_get_irq_chip_data(d);
> + struct regmap *regmap = priv->regmap;
> + int ret;
> +
> + ret = regmap_update_bits(regmap, AR9331_SW_REG_GINT_MASK,
> + AR9331_SW_GINT_PHY_INT, 0);
> + if (ret)
> + dev_err(priv->dev, "could not mask IRQ\n");
> +}
> +
> +static void ar9331_sw_unmask_irq(struct irq_data *d)
> +{
> + struct ar9331_sw_priv *priv = irq_data_get_irq_chip_data(d);
> + struct regmap *regmap = priv->regmap;
> + int ret;
> +
> + ret = regmap_update_bits(regmap, AR9331_SW_REG_GINT_MASK,
> + AR9331_SW_GINT_PHY_INT,
> + AR9331_SW_GINT_PHY_INT);
> + if (ret)
> + dev_err(priv->dev, "could not unmask IRQ\n");
> +}
> +
> +static struct irq_chip ar9331_sw_irq_chip = {
> + .name = AR9331_SW_NAME,
> + .irq_mask = ar9331_sw_mask_irq,
> + .irq_unmask = ar9331_sw_unmask_irq,
> +};
> +
> +static int ar9331_sw_irq_map(struct irq_domain *domain, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_data(irq, domain->host_data);
> + irq_set_chip_and_handler(irq, &ar9331_sw_irq_chip, handle_simple_irq);
> + irq_set_nested_thread(irq, 1);
> + irq_set_noprobe(irq);
> +
> + return 0;
> +}
> +
> +static void ar9331_sw_irq_unmap(struct irq_domain *d, unsigned int irq)
> +{
> + irq_set_nested_thread(irq, 0);
> + irq_set_chip_and_handler(irq, NULL, NULL);
> + irq_set_chip_data(irq, NULL);
> +}
> +
> +static const struct irq_domain_ops ar9331_sw_irqdomain_ops = {
> + .map = ar9331_sw_irq_map,
> + .unmap = ar9331_sw_irq_unmap,
> + .xlate = irq_domain_xlate_onecell,
> +};
> +
> +static int ar9331_sw_irq_init(struct ar9331_sw_priv *priv)
> +{
> + struct device_node *np = priv->dev->of_node;
> + struct device *dev = priv->dev;
> + int ret, irq;
> +
> + irq = of_irq_get(np, 0);
> + if (irq <= 0) {
> + dev_err(dev, "failed to get parent IRQ\n");
> + return irq ? irq : -EINVAL;
> + }
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL, ar9331_sw_irq,
> + IRQF_ONESHOT, AR9331_SW_NAME, priv);
> + if (ret) {
> + dev_err(dev, "unable to request irq: %d\n", ret);
> + return ret;
> + }
> +
> + priv->irqdomain = irq_domain_add_linear(np, 1, &ar9331_sw_irqdomain_ops,
> + priv);
> + if (!priv->irqdomain) {
> + dev_err(dev, "failed to create IRQ domain\n");
> + return -EINVAL;
> + }
> +
> + irq_set_parent(irq_create_mapping(priv->irqdomain, 0), irq);
> +
> + return 0;
> +}
> +
> +static int __ar9331_mdio_write(struct mii_bus *sbus, u8 mode, u16 reg, u16 val)
> +{
> + u8 r, p;
> +
> + p = FIELD_PREP(AR9331_SW_MDIO_PHY_MODE_M, mode) |
> + FIELD_GET(AR9331_SW_LOW_ADDR_PHY, reg);
> + r = FIELD_GET(AR9331_SW_LOW_ADDR_REG, reg);
> +
> + return sbus->write(sbus, p, r, val);

Why not use the mdiobus_write() and mdiobus_read()?

> +static int ar9331_sw_probe(struct mdio_device *mdiodev)
> +{
> + struct ar9331_sw_priv *priv;
> + int ret;
> +
> + /* allocate the private data struct so that we can probe the switches
> + * ID register
> + */

I don't see the code actually getting the ID register?


> + priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->regmap = devm_regmap_init(&mdiodev->dev, &ar9331_sw_bus, priv,
> + &ar9331_mdio_regmap_config);
> + if (IS_ERR(priv->regmap)) {
> + ret = PTR_ERR(priv->regmap);
> + dev_err(&mdiodev->dev, "regmap init failed: %d\n", ret);
> + return ret;
> + }
> +
> + priv->sw_reset = devm_reset_control_get(&mdiodev->dev, "switch");
> + if (IS_ERR(priv->sw_reset)) {
> + dev_err(&mdiodev->dev, "missing switch reset\n");
> + return PTR_ERR(priv->sw_reset);
> + }
> +
> + priv->sbus = mdiodev->bus;
> + priv->dev = &mdiodev->dev;
> +
> + ret = ar9331_sw_irq_init(priv);
> + if (ret)
> + return ret;
> +
> + priv->ds = dsa_switch_alloc(&mdiodev->dev, AR9331_SW_PORTS);
> + if (!priv->ds)
> + return -ENOMEM;
> +
> + priv->ds->priv = priv;
> + priv->ops = ar9331_sw_ops;
> + priv->ds->ops = &priv->ops;
> + dev_set_drvdata(&mdiodev->dev, priv);
> +
> + return dsa_register_switch(priv->ds);
> +}
> +
> +static void ar9331_sw_remove(struct mdio_device *mdiodev)
> +{
> + struct ar9331_sw_priv *priv = dev_get_drvdata(&mdiodev->dev);
> +
> + mdiobus_unregister(priv->mbus);
> + dsa_unregister_switch(priv->ds);
> +
> + reset_control_assert(priv->sw_reset);
> +}
> +
> +static const struct of_device_id ar9331_sw_of_match[] = {
> + { .compatible = "qca,ar9331-switch" },
> + { },
> +};
> +
> +static struct mdio_driver ar9331_sw_mdio_driver = {
> + .probe = ar9331_sw_probe,
> + .remove = ar9331_sw_remove,
> + .mdiodrv.driver = {
> + .name = AR9331_SW_NAME,
> + .of_match_table = ar9331_sw_of_match,
> + },
> +};
> +
> +mdio_module_driver(ar9331_sw_mdio_driver);
> +
> +MODULE_AUTHOR("Oleksij Rempel <[email protected]>");
> +MODULE_DESCRIPTION("Driver for Atheros AR9331 switch");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 541fb514e31d..89a334e68d42 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -42,6 +42,7 @@ struct phylink_link_state;
> #define DSA_TAG_PROTO_8021Q_VALUE 12
> #define DSA_TAG_PROTO_SJA1105_VALUE 13
> #define DSA_TAG_PROTO_KSZ8795_VALUE 14
> +#define DSA_TAG_PROTO_AR9331_VALUE 15
>
> enum dsa_tag_protocol {
> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
> @@ -59,6 +60,7 @@ enum dsa_tag_protocol {
> DSA_TAG_PROTO_8021Q = DSA_TAG_PROTO_8021Q_VALUE,
> DSA_TAG_PROTO_SJA1105 = DSA_TAG_PROTO_SJA1105_VALUE,
> DSA_TAG_PROTO_KSZ8795 = DSA_TAG_PROTO_KSZ8795_VALUE,
> + DSA_TAG_PROTO_AR9331 = DSA_TAG_PROTO_AR9331_VALUE,
> };
>
> struct packet_type;
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 29e2bd5cc5af..6e015309a7fe 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -107,4 +107,10 @@ config NET_DSA_TAG_TRAILER
> Say Y or M if you want to enable support for tagging frames at
> with a trailed. e.g. Marvell 88E6060.
>
> +config NET_DSA_TAG_AR9331
> + tristate "Tag driver for Atheros AR9331 SoC with build-in switch"
> + help
> + Say Y or M if you want to enable support for tagging frames for
> + the Atheros AR9331 SoC with build-in switch.
> +

These are somewhat sorted, based on the tristate string. So this
should go before NET_DSA_TAG_BRCM_COMMON.


> endif
> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> index 2c6d286f0511..67caebf602be 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
> obj-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o
> obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
> obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
> +obj-$(CONFIG_NET_DSA_TAG_AR9331) += tag_ar9331.o

Please keep with the sorting.

> diff --git a/net/dsa/tag_ar9331.c b/net/dsa/tag_ar9331.c
> new file mode 100644
> index 000000000000..b32a8d3d48b9
> --- /dev/null
> +++ b/net/dsa/tag_ar9331.c
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 Pengutronix, Oleksij Rempel <[email protected]>
> + */
> +
> +
> +#include <linux/bitfield.h>
> +#include <linux/etherdevice.h>
> +
> +#include "dsa_priv.h"
> +
> +#define AR9331_HDR_LEN 2
> +#define AR9331_HDR_VERSION 1
> +
> +#define AR9331_HDR_VERSION_MASK GENMASK(15, 14)
> +#define AR9331_HDR_PRIORITY_MASK GENMASK(13, 12)
> +#define AR9331_HDR_TYPE_MASK GENMASK(10, 8)
> +#define AR9331_HDR_BROADCAST BIT(7)
> +#define AR9331_HDR_FROM_CPU BIT(6)
> +/* AR9331_HDR_RESERVED - not used or may be version filed.

field

> + * According to the AR8216 doc it should 0b10. On AR9331 it is 0b11 on RX path
> + * and should be set to 0b11 to make it work.
> + */
> +#define AR9331_HDR_RESERVED_MASK GENMASK(5, 4)
> +#define AR9331_HDR_PORT_NUM_MASK GENMASK(3, 0)
> +
> +static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct dsa_port *dp = dsa_slave_to_port(dev);
> + __le16 *phdr;
> + u16 hdr;
> +
> + if (skb_cow_head(skb, 0) < 0)
> + return NULL;
> +
> + phdr = skb_push(skb, AR9331_HDR_LEN);
> +
> + hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);
> + hdr |= AR9331_HDR_FROM_CPU | dp->index;
> + /* 0b10 for AR8216 and 0b11 for AR9331 */
> + hdr |= AR9331_HDR_RESERVED_MASK;
> +
> + phdr[0] = cpu_to_le16(hdr);
> +
> + return skb;
> +}
> +
> +static struct sk_buff *ar9331_tag_rcv(struct sk_buff *skb,
> + struct net_device *ndev,
> + struct packet_type *pt)
> +{
> + u8 ver, port;
> + u16 hdr;
> +
> + if (unlikely(!pskb_may_pull(skb, AR9331_HDR_LEN)))
> + return NULL;
> +
> + hdr = le16_to_cpu(*(__le16 *)skb_mac_header(skb));
> +
> + ver = FIELD_GET(AR9331_HDR_VERSION_MASK, hdr);
> + if (unlikely(ver != AR9331_HDR_VERSION)) {
> + netdev_warn(ndev, "%s:%i wrong header version 0x%2x\n",
> + __func__, __LINE__, hdr);

This would should probably be rate limited.

> + return NULL;
> + }
> +
> + if (unlikely(hdr & AR9331_HDR_FROM_CPU)) {
> + netdev_warn(ndev, "%s:%i packet should not be from cpu 0x%2x\n",
> + __func__, __LINE__, hdr);

This as well.

Andrew

2019-10-16 16:12:56

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH v1 4/4] net: dsa: add support for Atheros AR9331 build-in switch

On Wed, Oct 16, 2019 at 03:00:57PM +0200, Andrew Lunn wrote:
> > +++ b/drivers/net/dsa/qca/ar9331.c
> > @@ -0,0 +1,822 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
>
> I think C files should use /* */, and header files //, for SPDX.

OK. In fact, there are both variants present in Kernel.
I'll do what ever format the maintainer think is acceptable.

> > +// Copyright (c) 2019 Pengutronix, Oleksij Rempel <[email protected]>
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <net/dsa.h>
> > +
> > +#define AR9331_SW_NAME "ar9331_switch"
> > +#define AR9331_SW_PORTS 6
> > +
> > +/* dummy reg to change page */
> > +#define AR9331_SW_REG_PAGE BIT(18)
> > +
> > +/* Global Interrupt */
> > +#define AR9331_SW_REG_GINT 0x10
> > +#define AR9331_SW_REG_GINT_MASK 0x14
> > +#define AR9331_SW_GINT_PHY_INT BIT(2)
> > +
> > +#define AR9331_SW_REG_FLOOD_MASK 0x2c
> > +#define AR9331_SW_FLOOD_MASK_BROAD_TO_CPU BIT(26)
> > +
> > +#define AR9331_SW_REG_GLOBAL_CTRL 0x30
> > +#define AR9331_SW_GLOBAL_CTRL_MFS_M GENMASK(13, 0)
> > +
> > +#define AR9331_SW_REG_MDIO_CTRL 0x98
> > +#define AR9331_SW_MDIO_CTRL_BUSY BIT(31)
> > +#define AR9331_SW_MDIO_CTRL_MASTER_EN BIT(30)
> > +#define AR9331_SW_MDIO_CTRL_CMD_READ BIT(27)
> > +#define AR9331_SW_MDIO_CTRL_PHY_ADDR_M GENMASK(25, 21)
> > +#define AR9331_SW_MDIO_CTRL_REG_ADDR_M GENMASK(20, 16)
> > +#define AR9331_SW_MDIO_CTRL_DATA_M GENMASK(16, 0)
> > +
> > +#define AR9331_SW_REG_PORT_STATUS(_port) (0x100 + (_port) * 0x100)
> > +
> > +/* FLOW_LINK_EN - enable mac flow control config auto-neg with phy.
> > + * If not set, mac can be config by software.
> > + */
> > +#define AR9331_SW_PORT_STATUS_FLOW_LINK_EN BIT(12)
> > +
> > +/* LINK_EN - If set, MAC is configured from PHY link status.
> > + * If not set, MAC should be configured by software.
> > + */
> > +#define AR9331_SW_PORT_STATUS_LINK_EN BIT(9)
> > +#define AR9331_SW_PORT_STATUS_DUPLEX_MODE BIT(6)
> > +#define AR9331_SW_PORT_STATUS_RX_FLOW_EN BIT(5)
> > +#define AR9331_SW_PORT_STATUS_TX_FLOW_EN BIT(4)
> > +#define AR9331_SW_PORT_STATUS_RXMAC BIT(3)
> > +#define AR9331_SW_PORT_STATUS_TXMAC BIT(2)
> > +#define AR9331_SW_PORT_STATUS_SPEED_M GENMASK(1, 0)
> > +#define AR9331_SW_PORT_STATUS_SPEED_1000 2
> > +#define AR9331_SW_PORT_STATUS_SPEED_100 1
> > +#define AR9331_SW_PORT_STATUS_SPEED_10 0
> > +
> > +#define AR9331_SW_PORT_STATUS_MAC_MASK \
> > + (AR9331_SW_PORT_STATUS_TXMAC | AR9331_SW_PORT_STATUS_RXMAC)
> > +
> > +#define AR9331_SW_PORT_STATUS_LINK_MASK \
> > + (AR9331_SW_PORT_STATUS_LINK_EN | AR9331_SW_PORT_STATUS_FLOW_LINK_EN | \
> > + AR9331_SW_PORT_STATUS_DUPLEX_MODE | \
> > + AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
> > + AR9331_SW_PORT_STATUS_SPEED_M)
> > +
> > +/* Phy bypass mode
> > + * ------------------------------------------------------------------------
> > + * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> > + *
> > + * real | start | OP | PhyAddr | Reg Addr | TA |
> > + * atheros| start | OP | 2'b00 |PhyAdd[2:0]| Reg Addr[4:0] | TA |
> > + *
> > + *
> > + * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
> > + * real | Data |
> > + * atheros| Data |
> > + *
> > + * ------------------------------------------------------------------------
> > + * Page address mode
> > + * ------------------------------------------------------------------------
> > + * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> > + * real | start | OP | PhyAddr | Reg Addr | TA |
> > + * atheros| start | OP | 2'b11 | 8'b0 | TA |
> > + *
> > + * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
> > + * real | Data |
> > + * atheros| | Page [9:0] |
> > + */
> > +/* In case of Page Address mode, Bit[18:9] of 32 bit register address should be
> > + * written to bits[9:0] of mdio data register.
> > + */
> > +#define AR9331_SW_ADDR_PAGE GENMASK(18, 9)
> > +
> > +/* ------------------------------------------------------------------------
> > + * Normal register access mode
> > + * ------------------------------------------------------------------------
> > + * Bit: | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> > + * real | start | OP | PhyAddr | Reg Addr | TA |
> > + * atheros| start | OP | 2'b10 | low_addr[7:0] | TA |
> > + *
> > + * Bit: |16 |17 |18 |19 |20 |21 |22 |23 |24 |25 |26 |27 |28 |29 |30 |31 |
> > + * real | Data |
> > + * atheros| Data |
> > + * ------------------------------------------------------------------------
> > + */
> > +#define AR9331_SW_LOW_ADDR_PHY GENMASK(8, 6)
> > +#define AR9331_SW_LOW_ADDR_REG GENMASK(5, 1)
> > +
> > +#define AR9331_SW_MDIO_PHY_MODE_M GENMASK(4, 3)
> > +#define AR9331_SW_MDIO_PHY_MODE_PAGE 3
> > +#define AR9331_SW_MDIO_PHY_MODE_REG 2
> > +#define AR9331_SW_MDIO_PHY_MODE_BYPASS 0
> > +#define AR9331_SW_MDIO_PHY_ADDR_M GENMASK(2, 0)
> > +
> > +/* Empirical determined values */
> > +#define AR9331_SW_MDIO_POLL_SLEEP_US 1
> > +#define AR9331_SW_MDIO_POLL_TIMEOUT_US 20
> > +
> > +struct ar9331_sw_priv {
> > + struct device *dev;
> > + struct dsa_switch *ds;
> > + struct dsa_switch_ops ops;
> > + struct irq_domain *irqdomain;
> > + struct mii_bus *mbus; /* mdio master */
> > + struct mii_bus *sbus; /* mdio slave */
> > + struct regmap *regmap;
> > + struct reset_control *sw_reset;
> > +};
> > +
> > +/* Warning: switch reset will reset last AR9331_SW_MDIO_PHY_MODE_PAGE request
> > + * If some kind of optimization is used, the request should be repeated.
> > + */
> > +static int ar9331_sw_reset(struct ar9331_sw_priv *priv)
> > +{
> > + int ret;
> > +
> > + ret = reset_control_assert(priv->sw_reset);
> > + if (ret)
> > + goto error;
> > +
> > + /* AR9331 doc do not provide any information about proper reset
> > + * sequence. The AR8136 (the closes switch to the AR9331) doc says:
> > + * reset duration should be greater than 10ms. So, let's use this value
> > + * for now.
> > + */
> > + usleep_range(10000, 15000);
> > + ret = reset_control_deassert(priv->sw_reset);
> > + if (ret)
> > + goto error;
>
> Any comments in the documentation about needing to wait for the reset
> to complete?

No. AR8136 has EEPROM_INT and EEPROM_ERR_INT for EEPROM load status,
which can be used as wait sequence after reset. But this is not present on
AR9331. As long as we support only AR9331 in this driver, looks like there is
no need to wait after reset.

Hm.. I think I'll add this comment to the code, just in case there is a
AR8136 switch out there on a system which can run modern linux kernel.

>
> > +
> > + return 0;
> > +error:
> > + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > + return ret;
> > +}
> > +
> > +static int ar9331_sw_mbus_write(struct mii_bus *mbus, int port, int regnum,
> > + u16 data)
> > +{
> > + struct ar9331_sw_priv *priv = mbus->priv;
> > + struct regmap *regmap = priv->regmap;
> > + u32 val;
> > + int ret;
> > +
> > + ret = regmap_write(regmap, AR9331_SW_REG_MDIO_CTRL,
> > + AR9331_SW_MDIO_CTRL_BUSY |
> > + AR9331_SW_MDIO_CTRL_MASTER_EN |
> > + FIELD_PREP(AR9331_SW_MDIO_CTRL_PHY_ADDR_M, port) |
> > + FIELD_PREP(AR9331_SW_MDIO_CTRL_REG_ADDR_M, regnum) |
> > + FIELD_PREP(AR9331_SW_MDIO_CTRL_DATA_M, data));
> > + if (ret)
> > + goto error;
> > +
> > + ret = regmap_read_poll_timeout(regmap, AR9331_SW_REG_MDIO_CTRL, val,
> > + !(val & AR9331_SW_MDIO_CTRL_BUSY),
> > + AR9331_SW_MDIO_POLL_SLEEP_US,
> > + AR9331_SW_MDIO_POLL_TIMEOUT_US);
> > + if (ret)
> > + goto error;
> > +
> > + return 0;
> > +error:
> > + dev_err_ratelimited(priv->dev, "PHY write error: %i\n", ret);
> > + return ret;
> > +}
> > +
> > +static int ar9331_sw_mbus_read(struct mii_bus *mbus, int port, int regnum)
> > +{
> > + struct ar9331_sw_priv *priv = mbus->priv;
> > + struct regmap *regmap = priv->regmap;
> > + u32 val;
> > + int ret;
> > +
> > + ret = regmap_write(regmap, AR9331_SW_REG_MDIO_CTRL,
> > + AR9331_SW_MDIO_CTRL_BUSY |
> > + AR9331_SW_MDIO_CTRL_MASTER_EN |
> > + AR9331_SW_MDIO_CTRL_CMD_READ |
> > + FIELD_PREP(AR9331_SW_MDIO_CTRL_PHY_ADDR_M, port) |
> > + FIELD_PREP(AR9331_SW_MDIO_CTRL_REG_ADDR_M, regnum));
> > + if (ret)
> > + goto error;
> > +
> > + ret = regmap_read_poll_timeout(regmap, AR9331_SW_REG_MDIO_CTRL, val,
> > + !(val & AR9331_SW_MDIO_CTRL_BUSY),
> > + AR9331_SW_MDIO_POLL_SLEEP_US,
> > + AR9331_SW_MDIO_POLL_TIMEOUT_US);
> > + if (ret)
> > + goto error;
> > +
> > + ret = regmap_read(regmap, AR9331_SW_REG_MDIO_CTRL, &val);
> > + if (ret)
> > + goto error;
> > +
> > + return FIELD_GET(AR9331_SW_MDIO_CTRL_DATA_M, val);
> > +
> > +error:
> > + dev_err_ratelimited(priv->dev, "PHY read error: %i\n", ret);
> > + return ret;
> > +}
> > +
> > +static int ar9331_sw_mbus_init(struct ar9331_sw_priv *priv)
> > +{
> > + struct device *dev = priv->dev;
> > + static struct mii_bus *mbus;
> > + struct device_node *np, *mnp;
> > + int ret;
> > +
> > + np = dev->of_node;
> > +
> > + mbus = devm_mdiobus_alloc(dev);
> > + if (!mbus)
> > + return -ENOMEM;
> > +
> > + mbus->name = np->full_name;
> > + snprintf(mbus->id, MII_BUS_ID_SIZE, "%pOF", np);
> > +
> > + mbus->read = ar9331_sw_mbus_read;
> > + mbus->write = ar9331_sw_mbus_write;
> > + mbus->priv = priv;
> > + mbus->parent = dev;
> > +
> > + mnp = of_get_child_by_name(np, "mdio");
>
> You should check if mnp is NULL. You want it to mandatory. The current
> code will pass NULL to of_mdiobus_register(), which is legal, and it
> will look one level higher for the PHYs.

ok

>
> > + ret = of_mdiobus_register(mbus, mnp);
> > + of_node_put(mnp);
> > + if (ret)
> > + return ret;
> > +
> > + priv->mbus = mbus;
> > +
> > + return 0;
> > +}
> > +
> > +static int ar9331_sw_setup(struct dsa_switch *ds)
> > +{
> > + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > +
> > + ret = ar9331_sw_reset(priv);
> > + if (ret)
> > + return ret;
> > +
> > + /* Reset will set proper defaults. CPU - Port0 will be enabled and
> > + * configured. All other ports (ports 1 - 5) are disabled
> > + */
>
> Nice, some hardware engineer thought about that.

:) is it not common case?

> > + ret = ar9331_sw_mbus_init(priv);
> > + if (ret)
> > + return ret;
> > +
> > + /* Do not drop broadcast frames */
> > + ret = regmap_write_bits(regmap, AR9331_SW_REG_FLOOD_MASK,
> > + AR9331_SW_FLOOD_MASK_BROAD_TO_CPU,
> > + AR9331_SW_FLOOD_MASK_BROAD_TO_CPU);
> > + if (ret)
> > + goto error;
> > +
> > + /* Sync max frame size with value used in
> > + * drivers/net/ethernet/atheros/ag71xx.c for ar9330 SoC.
> > + * TODO: In both drivers this value seems to be not real maximal size
> > + * The switch is able to configure 0x3fff and ethernet controller
> > + * 0xffff. Are there any better way to sync this values?
> > + */
> > + ret = regmap_write_bits(regmap, AR9331_SW_REG_GLOBAL_CTRL,
> > + AR9331_SW_GLOBAL_CTRL_MFS_M,
> > + FIELD_PREP(AR9331_SW_GLOBAL_CTRL_MFS_M, 1540));
> > + if (ret)
> > + goto error;
>
> Jumbo is not so easy. I would avoid the plain number 1540. There
> should be a #define.

ok

> Also, you might want to allow space for a VLAN header?

yes.

> Does enabbling jumbo have a performance impact? If not, you
> can configure the switch to its maximum size.

ok.

> > +
> > + return 0;
> > +error:
> > + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > + return ret;
> > +}
> > +
> > +static int ar9331_sw_port_enable(struct dsa_switch *ds, int port,
> > + struct phy_device *phy)
> > +{
> > + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > +
> > + /* nothing to enable. Just set link to initial state */
> > + ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
> > + if (ret)
> > + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static void ar9331_sw_port_disable(struct dsa_switch *ds, int port)
> > +{
> > + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > +
> > + ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
> > + if (ret)
> > + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > +}
>
> port_enable() and port_disable() look the same?

yes. there only two bits in port register: tx and rx mac enable. So, I
was not able to decide where to set this bits - In link_up/down or
port_up/down. I decided to make sure, port up/down should have always
predictable configuration: tx/rx mac is off and automatic
negation is disabled.

What is the better way?

> > +
> > +static enum dsa_tag_protocol ar9331_sw_get_tag_protocol(struct dsa_switch *ds,
> > + int port)
> > +{
> > + return DSA_TAG_PROTO_AR9331;
> > +}
> > +
> > +static void ar9331_sw_phylink_validate(struct dsa_switch *ds, int port,
> > + unsigned long *supported,
> > + struct phylink_link_state *state)
> > +{
> > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > + switch (port) {
> > + case 0:
> > + if (state->interface != PHY_INTERFACE_MODE_GMII)
> > + goto unsupported;
> > +
> > + phylink_set(mask, 1000baseT_Full);
> > + phylink_set(mask, 1000baseT_Half);
> > + break;
> > + case 1:
> > + case 2:
> > + case 3:
> > + case 4:
> > + case 5:
> > + if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > + goto unsupported;
> > + break;
> > + default:
> > + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> > + dev_err(ds->dev, "Unsupported port: %i\n", port);
> > + return;
> > + }
> > +
> > + phylink_set_port_modes(mask);
> > + phylink_set(mask, Pause);
> > + phylink_set(mask, Asym_Pause);
> > +
> > + phylink_set(mask, 10baseT_Half);
> > + phylink_set(mask, 10baseT_Full);
> > + phylink_set(mask, 100baseT_Half);
> > + phylink_set(mask, 100baseT_Full);
>
> So the CPU port is 1G capable. All the other ports are only Fast Ethernet?

yes.

> > +
> > + bitmap_and(supported, supported, mask,
> > + __ETHTOOL_LINK_MODE_MASK_NBITS);
> > + bitmap_and(state->advertising, state->advertising, mask,
> > + __ETHTOOL_LINK_MODE_MASK_NBITS);
> > +
> > + return;
> > +
> > +unsupported:
> > + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> > + dev_err(ds->dev, "Unsupported interface: %d, port: %d\n",
> > + state->interface, port);
> > +}
> > +
> > +static void ar9331_sw_phylink_mac_config(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + const struct phylink_link_state *state)
> > +{
> > + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > + u32 val;
> > +
> > + switch (state->speed) {
> > + case SPEED_1000:
> > + val = AR9331_SW_PORT_STATUS_SPEED_1000;
> > + break;
> > + case SPEED_100:
> > + val = AR9331_SW_PORT_STATUS_SPEED_100;
> > + break;
> > + case SPEED_10:
> > + val = AR9331_SW_PORT_STATUS_SPEED_10;
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + if (state->duplex)
> > + val |= AR9331_SW_PORT_STATUS_DUPLEX_MODE;
> > +
> > + if (state->pause & MLO_PAUSE_TX)
> > + val |= AR9331_SW_PORT_STATUS_TX_FLOW_EN;
> > +
> > + if (state->pause & MLO_PAUSE_RX)
> > + val |= AR9331_SW_PORT_STATUS_RX_FLOW_EN;
> > +
> > + ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
> > + AR9331_SW_PORT_STATUS_LINK_MASK, val);
> > + if (ret)
> > + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > +}
> > +
> > +static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + phy_interface_t interface)
> > +{
> > + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > +
> > + ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
> > + AR9331_SW_PORT_STATUS_MAC_MASK, 0);
> > + if (ret)
> > + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > +}
> > +
> > +static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + phy_interface_t interface,
> > + struct phy_device *phydev)
> > +{
> > + struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > +
> > + ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_STATUS(port),
> > + AR9331_SW_PORT_STATUS_MAC_MASK,
> > + AR9331_SW_PORT_STATUS_MAC_MASK);
> > + if (ret)
> > + dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > +}
> > +
> > +static const struct dsa_switch_ops ar9331_sw_ops = {
> > + .get_tag_protocol = ar9331_sw_get_tag_protocol,
> > + .setup = ar9331_sw_setup,
> > + .port_enable = ar9331_sw_port_enable,
> > + .port_disable = ar9331_sw_port_disable,
> > + .phylink_validate = ar9331_sw_phylink_validate,
> > + .phylink_mac_config = ar9331_sw_phylink_mac_config,
> > + .phylink_mac_link_down = ar9331_sw_phylink_mac_link_down,
> > + .phylink_mac_link_up = ar9331_sw_phylink_mac_link_up,
> > +};
> > +
> > +static irqreturn_t ar9331_sw_irq(int irq, void *data)
> > +{
> > + struct ar9331_sw_priv *priv = data;
> > + struct regmap *regmap = priv->regmap;
> > + u32 stat;
> > + int ret;
> > +
> > + ret = regmap_read(regmap, AR9331_SW_REG_GINT, &stat);
> > + if (ret) {
> > + dev_err(priv->dev, "can't read interrupt status\n");
> > + return IRQ_NONE;
> > + }
> > +
> > + if (!stat)
> > + return IRQ_NONE;
> > +
> > + if (stat & AR9331_SW_GINT_PHY_INT) {
> > + int child_irq;
> > +
> > + child_irq = irq_find_mapping(priv->irqdomain, 0);
> > + handle_nested_irq(child_irq);
> > + }
> > +
> > + ret = regmap_write(regmap, AR9331_SW_REG_GINT, stat);
> > + if (ret) {
> > + dev_err(priv->dev, "can't write interrupt status\n");
> > + return IRQ_NONE;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void ar9331_sw_mask_irq(struct irq_data *d)
> > +{
> > + struct ar9331_sw_priv *priv = irq_data_get_irq_chip_data(d);
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > +
> > + ret = regmap_update_bits(regmap, AR9331_SW_REG_GINT_MASK,
> > + AR9331_SW_GINT_PHY_INT, 0);
> > + if (ret)
> > + dev_err(priv->dev, "could not mask IRQ\n");
> > +}
> > +
> > +static void ar9331_sw_unmask_irq(struct irq_data *d)
> > +{
> > + struct ar9331_sw_priv *priv = irq_data_get_irq_chip_data(d);
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > +
> > + ret = regmap_update_bits(regmap, AR9331_SW_REG_GINT_MASK,
> > + AR9331_SW_GINT_PHY_INT,
> > + AR9331_SW_GINT_PHY_INT);
> > + if (ret)
> > + dev_err(priv->dev, "could not unmask IRQ\n");
> > +}
> > +
> > +static struct irq_chip ar9331_sw_irq_chip = {
> > + .name = AR9331_SW_NAME,
> > + .irq_mask = ar9331_sw_mask_irq,
> > + .irq_unmask = ar9331_sw_unmask_irq,
> > +};
> > +
> > +static int ar9331_sw_irq_map(struct irq_domain *domain, unsigned int irq,
> > + irq_hw_number_t hwirq)
> > +{
> > + irq_set_chip_data(irq, domain->host_data);
> > + irq_set_chip_and_handler(irq, &ar9331_sw_irq_chip, handle_simple_irq);
> > + irq_set_nested_thread(irq, 1);
> > + irq_set_noprobe(irq);
> > +
> > + return 0;
> > +}
> > +
> > +static void ar9331_sw_irq_unmap(struct irq_domain *d, unsigned int irq)
> > +{
> > + irq_set_nested_thread(irq, 0);
> > + irq_set_chip_and_handler(irq, NULL, NULL);
> > + irq_set_chip_data(irq, NULL);
> > +}
> > +
> > +static const struct irq_domain_ops ar9331_sw_irqdomain_ops = {
> > + .map = ar9331_sw_irq_map,
> > + .unmap = ar9331_sw_irq_unmap,
> > + .xlate = irq_domain_xlate_onecell,
> > +};
> > +
> > +static int ar9331_sw_irq_init(struct ar9331_sw_priv *priv)
> > +{
> > + struct device_node *np = priv->dev->of_node;
> > + struct device *dev = priv->dev;
> > + int ret, irq;
> > +
> > + irq = of_irq_get(np, 0);
> > + if (irq <= 0) {
> > + dev_err(dev, "failed to get parent IRQ\n");
> > + return irq ? irq : -EINVAL;
> > + }
> > +
> > + ret = devm_request_threaded_irq(dev, irq, NULL, ar9331_sw_irq,
> > + IRQF_ONESHOT, AR9331_SW_NAME, priv);
> > + if (ret) {
> > + dev_err(dev, "unable to request irq: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + priv->irqdomain = irq_domain_add_linear(np, 1, &ar9331_sw_irqdomain_ops,
> > + priv);
> > + if (!priv->irqdomain) {
> > + dev_err(dev, "failed to create IRQ domain\n");
> > + return -EINVAL;
> > + }
> > +
> > + irq_set_parent(irq_create_mapping(priv->irqdomain, 0), irq);
> > +
> > + return 0;
> > +}
> > +
> > +static int __ar9331_mdio_write(struct mii_bus *sbus, u8 mode, u16 reg, u16 val)
> > +{
> > + u8 r, p;
> > +
> > + p = FIELD_PREP(AR9331_SW_MDIO_PHY_MODE_M, mode) |
> > + FIELD_GET(AR9331_SW_LOW_ADDR_PHY, reg);
> > + r = FIELD_GET(AR9331_SW_LOW_ADDR_REG, reg);
> > +
> > + return sbus->write(sbus, p, r, val);
>
> Why not use the mdiobus_write() and mdiobus_read()?

no special reason. I'll fix.

> > +static int ar9331_sw_probe(struct mdio_device *mdiodev)
> > +{
> > + struct ar9331_sw_priv *priv;
> > + int ret;
> > +
> > + /* allocate the private data struct so that we can probe the switches
> > + * ID register
> > + */
>
> I don't see the code actually getting the ID register?

old artifact, i'll remove it. ID bits are actually not documented.


> > + priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->regmap = devm_regmap_init(&mdiodev->dev, &ar9331_sw_bus, priv,
> > + &ar9331_mdio_regmap_config);
> > + if (IS_ERR(priv->regmap)) {
> > + ret = PTR_ERR(priv->regmap);
> > + dev_err(&mdiodev->dev, "regmap init failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + priv->sw_reset = devm_reset_control_get(&mdiodev->dev, "switch");
> > + if (IS_ERR(priv->sw_reset)) {
> > + dev_err(&mdiodev->dev, "missing switch reset\n");
> > + return PTR_ERR(priv->sw_reset);
> > + }
> > +
> > + priv->sbus = mdiodev->bus;
> > + priv->dev = &mdiodev->dev;
> > +
> > + ret = ar9331_sw_irq_init(priv);
> > + if (ret)
> > + return ret;
> > +
> > + priv->ds = dsa_switch_alloc(&mdiodev->dev, AR9331_SW_PORTS);
> > + if (!priv->ds)
> > + return -ENOMEM;
> > +
> > + priv->ds->priv = priv;
> > + priv->ops = ar9331_sw_ops;
> > + priv->ds->ops = &priv->ops;
> > + dev_set_drvdata(&mdiodev->dev, priv);
> > +
> > + return dsa_register_switch(priv->ds);
> > +}
> > +
> > +static void ar9331_sw_remove(struct mdio_device *mdiodev)
> > +{
> > + struct ar9331_sw_priv *priv = dev_get_drvdata(&mdiodev->dev);
> > +
> > + mdiobus_unregister(priv->mbus);
> > + dsa_unregister_switch(priv->ds);
> > +
> > + reset_control_assert(priv->sw_reset);
> > +}
> > +
> > +static const struct of_device_id ar9331_sw_of_match[] = {
> > + { .compatible = "qca,ar9331-switch" },
> > + { },
> > +};
> > +
> > +static struct mdio_driver ar9331_sw_mdio_driver = {
> > + .probe = ar9331_sw_probe,
> > + .remove = ar9331_sw_remove,
> > + .mdiodrv.driver = {
> > + .name = AR9331_SW_NAME,
> > + .of_match_table = ar9331_sw_of_match,
> > + },
> > +};
> > +
> > +mdio_module_driver(ar9331_sw_mdio_driver);
> > +
> > +MODULE_AUTHOR("Oleksij Rempel <[email protected]>");
> > +MODULE_DESCRIPTION("Driver for Atheros AR9331 switch");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index 541fb514e31d..89a334e68d42 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -42,6 +42,7 @@ struct phylink_link_state;
> > #define DSA_TAG_PROTO_8021Q_VALUE 12
> > #define DSA_TAG_PROTO_SJA1105_VALUE 13
> > #define DSA_TAG_PROTO_KSZ8795_VALUE 14
> > +#define DSA_TAG_PROTO_AR9331_VALUE 15
> >
> > enum dsa_tag_protocol {
> > DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
> > @@ -59,6 +60,7 @@ enum dsa_tag_protocol {
> > DSA_TAG_PROTO_8021Q = DSA_TAG_PROTO_8021Q_VALUE,
> > DSA_TAG_PROTO_SJA1105 = DSA_TAG_PROTO_SJA1105_VALUE,
> > DSA_TAG_PROTO_KSZ8795 = DSA_TAG_PROTO_KSZ8795_VALUE,
> > + DSA_TAG_PROTO_AR9331 = DSA_TAG_PROTO_AR9331_VALUE,
> > };
> >
> > struct packet_type;
> > diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> > index 29e2bd5cc5af..6e015309a7fe 100644
> > --- a/net/dsa/Kconfig
> > +++ b/net/dsa/Kconfig
> > @@ -107,4 +107,10 @@ config NET_DSA_TAG_TRAILER
> > Say Y or M if you want to enable support for tagging frames at
> > with a trailed. e.g. Marvell 88E6060.
> >
> > +config NET_DSA_TAG_AR9331
> > + tristate "Tag driver for Atheros AR9331 SoC with build-in switch"
> > + help
> > + Say Y or M if you want to enable support for tagging frames for
> > + the Atheros AR9331 SoC with build-in switch.
> > +
>
> These are somewhat sorted, based on the tristate string. So this
> should go before NET_DSA_TAG_BRCM_COMMON.

ok

>
> > endif
> > diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> > index 2c6d286f0511..67caebf602be 100644
> > --- a/net/dsa/Makefile
> > +++ b/net/dsa/Makefile
> > @@ -15,3 +15,4 @@ obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
> > obj-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o
> > obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
> > obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
> > +obj-$(CONFIG_NET_DSA_TAG_AR9331) += tag_ar9331.o
>
> Please keep with the sorting.

ok

> > diff --git a/net/dsa/tag_ar9331.c b/net/dsa/tag_ar9331.c
> > new file mode 100644
> > index 000000000000..b32a8d3d48b9
> > --- /dev/null
> > +++ b/net/dsa/tag_ar9331.c
> > @@ -0,0 +1,97 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019 Pengutronix, Oleksij Rempel <[email protected]>
> > + */
> > +
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/etherdevice.h>
> > +
> > +#include "dsa_priv.h"
> > +
> > +#define AR9331_HDR_LEN 2
> > +#define AR9331_HDR_VERSION 1
> > +
> > +#define AR9331_HDR_VERSION_MASK GENMASK(15, 14)
> > +#define AR9331_HDR_PRIORITY_MASK GENMASK(13, 12)
> > +#define AR9331_HDR_TYPE_MASK GENMASK(10, 8)
> > +#define AR9331_HDR_BROADCAST BIT(7)
> > +#define AR9331_HDR_FROM_CPU BIT(6)
> > +/* AR9331_HDR_RESERVED - not used or may be version filed.
>
> field

thx. ok

> > + * According to the AR8216 doc it should 0b10. On AR9331 it is 0b11 on RX path
> > + * and should be set to 0b11 to make it work.
> > + */
> > +#define AR9331_HDR_RESERVED_MASK GENMASK(5, 4)
> > +#define AR9331_HDR_PORT_NUM_MASK GENMASK(3, 0)
> > +
> > +static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
> > + struct net_device *dev)
> > +{
> > + struct dsa_port *dp = dsa_slave_to_port(dev);
> > + __le16 *phdr;
> > + u16 hdr;
> > +
> > + if (skb_cow_head(skb, 0) < 0)
> > + return NULL;
> > +
> > + phdr = skb_push(skb, AR9331_HDR_LEN);
> > +
> > + hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);
> > + hdr |= AR9331_HDR_FROM_CPU | dp->index;
> > + /* 0b10 for AR8216 and 0b11 for AR9331 */
> > + hdr |= AR9331_HDR_RESERVED_MASK;
> > +
> > + phdr[0] = cpu_to_le16(hdr);
> > +
> > + return skb;
> > +}
> > +
> > +static struct sk_buff *ar9331_tag_rcv(struct sk_buff *skb,
> > + struct net_device *ndev,
> > + struct packet_type *pt)
> > +{
> > + u8 ver, port;
> > + u16 hdr;
> > +
> > + if (unlikely(!pskb_may_pull(skb, AR9331_HDR_LEN)))
> > + return NULL;
> > +
> > + hdr = le16_to_cpu(*(__le16 *)skb_mac_header(skb));
> > +
> > + ver = FIELD_GET(AR9331_HDR_VERSION_MASK, hdr);
> > + if (unlikely(ver != AR9331_HDR_VERSION)) {
> > + netdev_warn(ndev, "%s:%i wrong header version 0x%2x\n",
> > + __func__, __LINE__, hdr);
>
> This would should probably be rate limited.

good point. ok

> > + return NULL;
> > + }
> > +
> > + if (unlikely(hdr & AR9331_HDR_FROM_CPU)) {
> > + netdev_warn(ndev, "%s:%i packet should not be from cpu 0x%2x\n",
> > + __func__, __LINE__, hdr);
>
> This as well.

ok.

> Andrew
>

Regards,
Oleksij
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2019-10-17 11:03:17

by Fabio Estevam

[permalink] [raw]
Subject: Re: [PATCH v1 4/4] net: dsa: add support for Atheros AR9331 build-in switch

On Wed, Oct 16, 2019 at 1:01 PM Andrew Lunn <[email protected]> wrote:

> I think C files should use /* */, and header files //, for SPDX.

Not really.

From Documentation/process/license-rules.rst:

"C source: // SPDX-License-Identifier: <SPDX License Expression>
C header: /* SPDX-License-Identifier: <SPDX License Expression> */ "

2019-10-17 13:37:53

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v1 2/4] dt-bindings: net: dsa: qca,ar9331 switch documentation

On Mon, Oct 14, 2019 at 08:15:47AM +0200, Oleksij Rempel wrote:
> Atheros AR9331 has built-in 5 port switch. The switch can be configured
> to use all 5 or 4 ports. One of built-in PHYs can be used by first built-in
> ethernet controller or to be used directly by the switch over second ethernet
> controller.

Hi Oleksij

How exactly is this phy sharing controlled? I did not see anything in
the driver. Is there a mux we need to set?

Andrew

2019-10-18 09:51:57

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH v1 2/4] dt-bindings: net: dsa: qca,ar9331 switch documentation



On 16.10.19 22:23, Andrew Lunn wrote:
> On Mon, Oct 14, 2019 at 08:15:47AM +0200, Oleksij Rempel wrote:
>> Atheros AR9331 has built-in 5 port switch. The switch can be configured
>> to use all 5 or 4 ports. One of built-in PHYs can be used by first built-in
>> ethernet controller or to be used directly by the switch over second ethernet
>> controller.
>
> Hi Oleksij
>
> How exactly is this phy sharing controlled? I did not see anything in
> the driver. Is there a mux we need to set?

Currently it is not controlled at all, eth0 should be disabled and switch port5 enabled
(or other way around) in devicetree. If both are enabled, it will be some how brocken. I
don't know how to properly implement it.
I assume, it should not be controlled by devicetree configuration and user should be able
to do it dynamically from user space.

Ideas, suggestions?

Kind regards,
Oleksij Rempel

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2019-10-18 22:16:21

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH v1 4/4] net: dsa: add support for Atheros AR9331 build-in switch



On 10/13/2019 11:15 PM, Oleksij Rempel wrote:
> Provide basic support for Atheros AR9331 build-in switch. So far it
> works as port multiplexer without any hardware offloading support.

I glanced through the functional parts of the code, and it looks pretty
straight forward, since there is no offloading done so far, do you plan
on adding bridge offload eventually if nothing more?

When you submit v2, I would suggest splitting the tagger code from the
switch driver code, just to make them easier to review.

Thanks!
--
Florian

2019-10-18 22:33:06

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH v1 4/4] net: dsa: add support for Atheros AR9331 build-in switch

On Thu, Oct 17, 2019 at 11:35:48AM -0700, Florian Fainelli wrote:
>
>
> On 10/13/2019 11:15 PM, Oleksij Rempel wrote:
> > Provide basic support for Atheros AR9331 build-in switch. So far it
> > works as port multiplexer without any hardware offloading support.
>
> I glanced through the functional parts of the code, and it looks pretty
> straight forward, since there is no offloading done so far, do you plan
> on adding bridge offload eventually if nothing more?

Currently not. There are following reasons:
- I do it for the Freifunk project. It is currently not clear, what
functionality has higher priority.
- there are not many ar9331 based devices with enough RAM to run any
thing modern. There is even less devices using more then one switch
port.
- IPv6 support is important for this project, but old Atheros switches have some
known issues with IPv6 packages in hardware bridge mode. So, this
functionality will need more testing.

> When you submit v2, I would suggest splitting the tagger code from the
> switch driver code, just to make them easier to review.

ok.

Regards,
Oleksij
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |