This patch replaces the adjust_link api with the phylink apis that provide
equivalent functionality.
The remaining functionality from the adjust_link is now covered in the
phylink_mac_link_* and phylink_mac_config.
Removes:
.adjust_link
Adds:
.phylink_get_caps
.phylink_mac_link_down
.phylink_mac_link_up
.phylink_mac_link_down
Signed-off-by: Pawel Dembicki <[email protected]>
---
drivers/net/dsa/vitesse-vsc73xx-core.c | 179 ++++++++++++++-----------
1 file changed, 99 insertions(+), 80 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index ae55167ce0a6..e853b57b0bc8 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -39,6 +39,7 @@
#define VSC73XX_BLOCK_SYSTEM 0x7 /* Only subblock 0 */
#define CPU_PORT 6 /* CPU port */
+#define VSC73XX_TABLE_ATTEMPTS 10
/* MAC Block registers */
#define VSC73XX_MAC_CFG 0x00
@@ -715,8 +716,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
}
static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
- int port, struct phy_device *phydev,
- u32 initval)
+ int port, u32 initval)
{
u32 val = initval;
u8 seed;
@@ -754,12 +754,40 @@ static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN);
}
-static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
- struct phy_device *phydev)
+static void vsc73xx_phylink_get_caps(struct dsa_switch *ds, int port,
+ struct phylink_config *config)
{
- struct vsc73xx *vsc = ds->priv;
- u32 val;
+ /* This switch only supports full-duplex at 1Gbps */
+ config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000FD |
+ MAC_ASYM_PAUSE | MAC_SYM_PAUSE;
+ if (port == CPU_PORT) {
+ __set_bit(PHY_INTERFACE_MODE_RGMII,
+ config->supported_interfaces);
+ __set_bit(PHY_INTERFACE_MODE_GMII,
+ config->supported_interfaces);
+ } else {
+ __set_bit(PHY_INTERFACE_MODE_INTERNAL,
+ config->supported_interfaces);
+ /* Compatibility for phylib's default interface type when the
+ * phy-mode property is absent
+ */
+ __set_bit(PHY_INTERFACE_MODE_GMII,
+ config->supported_interfaces);
+ }
+
+ /* This driver does not make use of the speed, duplex, pause or the
+ * advertisement in its mac_config, so it is safe to mark this driver
+ * as non-legacy.
+ */
+ config->legacy_pre_march2020 = false;
+}
+
+static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ const struct phylink_link_state *state)
+{
+ struct vsc73xx *vsc = ds->priv;
/* Special handling of the CPU-facing port */
if (port == CPU_PORT) {
/* Other ports are already initialized but not this one */
@@ -775,104 +803,92 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
VSC73XX_ADVPORTM_ENA_GTX |
VSC73XX_ADVPORTM_DDR_MODE);
}
+}
- /* This is the MAC confiuration that always need to happen
- * after a PHY or the CPU port comes up or down.
- */
- if (!phydev->link) {
- int maxloop = 10;
+static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface)
+{
+ struct vsc73xx *vsc = ds->priv;
+ u32 val;
- dev_dbg(vsc->dev, "port %d: went down\n",
- port);
+ int maxloop = VSC73XX_TABLE_ATTEMPTS;
- /* Disable RX on this port */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
- VSC73XX_MAC_CFG,
- VSC73XX_MAC_CFG_RX_EN, 0);
+ dev_dbg(vsc->dev, "port %d: went down\n",
+ port);
- /* Discard packets */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBDISC, BIT(port), BIT(port));
+ /* Disable RX on this port */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_MAC_CFG,
+ VSC73XX_MAC_CFG_RX_EN, 0);
+
+ /* Discard packets */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_ARBDISC, BIT(port), BIT(port));
- /* Wait until queue is empty */
+ /* Wait until queue is empty */
+ vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_ARBEMPTY, &val);
+ while (!(val & BIT(port))) {
+ msleep(1);
vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
VSC73XX_ARBEMPTY, &val);
- while (!(val & BIT(port))) {
- msleep(1);
- vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBEMPTY, &val);
- if (--maxloop == 0) {
- dev_err(vsc->dev,
- "timeout waiting for block arbiter\n");
- /* Continue anyway */
- break;
- }
+ if (--maxloop == 0) {
+ dev_err(vsc->dev,
+ "timeout waiting for block arbiter\n");
+ /* Continue anyway */
+ break;
}
+ }
- /* Put this port into reset */
- vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
- VSC73XX_MAC_CFG_RESET);
-
- /* Accept packets again */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBDISC, BIT(port), 0);
+ /* Put this port into reset */
+ vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
+ VSC73XX_MAC_CFG_RESET);
- /* Allow backward dropping of frames from this port */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_SBACKWDROP, BIT(port), BIT(port));
+ /* Accept packets again */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_ARBDISC, BIT(port), 0);
- /* Receive mask (disable forwarding) */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
- VSC73XX_RECVMASK, BIT(port), 0);
+ /* Allow backward dropping of frames from this port */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_SBACKWDROP, BIT(port), BIT(port));
- return;
- }
+ /* Receive mask (disable forwarding) */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_RECVMASK, BIT(port), 0);
+}
- /* Figure out what speed was negotiated */
- if (phydev->speed == SPEED_1000) {
- dev_dbg(vsc->dev, "port %d: 1000 Mbit mode full duplex\n",
- port);
+static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface,
+ struct phy_device *phydev,
+ int speed, int duplex,
+ bool tx_pause, bool rx_pause)
+{
+ struct vsc73xx *vsc = ds->priv;
+ u32 val;
+ switch (speed) {
+ case SPEED_1000:
/* Set up default for internal port or external RGMII */
- if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
+ if (interface == PHY_INTERFACE_MODE_RGMII)
val = VSC73XX_MAC_CFG_1000M_F_RGMII;
else
val = VSC73XX_MAC_CFG_1000M_F_PHY;
- vsc73xx_adjust_enable_port(vsc, port, phydev, val);
- } else if (phydev->speed == SPEED_100) {
- if (phydev->duplex == DUPLEX_FULL) {
- val = VSC73XX_MAC_CFG_100_10M_F_PHY;
- dev_dbg(vsc->dev,
- "port %d: 100 Mbit full duplex mode\n",
- port);
- } else {
- val = VSC73XX_MAC_CFG_100_10M_H_PHY;
- dev_dbg(vsc->dev,
- "port %d: 100 Mbit half duplex mode\n",
- port);
- }
- vsc73xx_adjust_enable_port(vsc, port, phydev, val);
- } else if (phydev->speed == SPEED_10) {
- if (phydev->duplex == DUPLEX_FULL) {
+ break;
+ case SPEED_100:
+ case SPEED_10:
+ if (duplex == DUPLEX_FULL)
val = VSC73XX_MAC_CFG_100_10M_F_PHY;
- dev_dbg(vsc->dev,
- "port %d: 10 Mbit full duplex mode\n",
- port);
- } else {
+ else
val = VSC73XX_MAC_CFG_100_10M_H_PHY;
- dev_dbg(vsc->dev,
- "port %d: 10 Mbit half duplex mode\n",
- port);
- }
- vsc73xx_adjust_enable_port(vsc, port, phydev, val);
- } else {
- dev_err(vsc->dev,
- "could not adjust link: unknown speed\n");
+ break;
}
/* Enable port (forwarding) in the receieve mask */
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
VSC73XX_RECVMASK, BIT(port), BIT(port));
+ vsc73xx_adjust_enable_port(vsc, port, val);
}
static int vsc73xx_port_enable(struct dsa_switch *ds, int port,
@@ -1043,7 +1059,10 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.setup = vsc73xx_setup,
.phy_read = vsc73xx_phy_read,
.phy_write = vsc73xx_phy_write,
- .adjust_link = vsc73xx_adjust_link,
+ .phylink_get_caps = vsc73xx_phylink_get_caps,
+ .phylink_mac_config = vsc73xx_phylink_mac_config,
+ .phylink_mac_link_down = vsc73xx_phylink_mac_link_down,
+ .phylink_mac_link_up = vsc73xx_phylink_mac_link_up,
.get_strings = vsc73xx_get_strings,
.get_ethtool_stats = vsc73xx_get_ethtool_stats,
.get_sset_count = vsc73xx_get_sset_count,
--
2.34.1
This patch implement vlan filtering for vsc73xx driver.
After vlan filtering start, switch is reconfigured from QinQ to simple
vlan aware mode. It's required, because VSC73XX chips haven't support
for inner vlan tag filter.
Signed-off-by: Pawel Dembicki <[email protected]>
---
drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
1 file changed, 101 insertions(+)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 427b6f964811..fcce47cf6da4 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1239,6 +1239,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
return ret;
}
+static int
+vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
+ bool vlan_filtering, struct netlink_ext_ack *extack)
+{
+ int ret, i;
+
+ if (vlan_filtering) {
+ vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE);
+ } else {
+ if (port == CPU_PORT)
+ vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE);
+ else
+ vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE);
+ }
+
+ for (i = 0; i <= 3072; i++) {
+ ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
bool port_vlan)
{
@@ -1317,6 +1341,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
return 0;
}
+static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan,
+ struct netlink_ext_ack *extack)
+{
+ bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+ bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+ int ret;
+
+ /* Be sure to deny alterations to the configuration done by tag_8021q.
+ */
+ if (vid_is_dsa_8021q(vlan->vid)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Range 3072-4095 reserved for dsa_8021q operation");
+ return -EBUSY;
+ }
+
+ if (untagged && port != CPU_PORT) {
+ ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
+ if (ret)
+ return ret;
+ }
+ if (pvid && port != CPU_PORT) {
+ ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
+ if (ret)
+ return ret;
+ }
+
+ ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1);
+
+ return ret;
+}
+
+static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan)
+{
+ struct vsc73xx *vsc = ds->priv;
+ u16 vlan_no;
+ int ret;
+ u32 val;
+
+ ret =
+ vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0);
+ if (ret)
+ return ret;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
+
+ if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) {
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG, &val);
+ vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
+ if (vlan_no == vlan->vid) {
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
+ 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
+ }
+ }
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
+ vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
+ if (vlan_no && vlan_no == vlan->vid) {
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_CAT_PORT_VLAN,
+ VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
+ }
+
+ return 0;
+}
+
static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
{
int i;
@@ -1537,6 +1635,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.port_change_mtu = vsc73xx_change_mtu,
.port_max_mtu = vsc73xx_get_max_mtu,
.port_stp_state_set = vsc73xx_port_stp_state_set,
+ .port_vlan_filtering = vsc73xx_port_vlan_filtering,
+ .port_vlan_add = vsc73xx_port_vlan_add,
+ .port_vlan_del = vsc73xx_port_vlan_del,
.tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
.tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
};
--
2.34.1
This patch adds bridge support for vsc73xx driver.
It introduce two functions for port_bridge_join and
vsc73xx_port_bridge_leave handling.
Those functions implement forwarding adjust and use
dsa_tag_8021q_bridge_* api for adjust VLAN configuration.
Signed-off-by: Pawel Dembicki <[email protected]>
---
drivers/net/dsa/vitesse-vsc73xx-core.c | 69 ++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 653914fb5796..427b6f964811 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1317,6 +1317,72 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
return 0;
}
+static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
+{
+ int i;
+
+ for (i = 0; i < vsc->ds->num_ports; i++) {
+ u32 val;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + i, &val);
+ /* update only if port is in forwarding state*/
+ if (val & VSC73XX_SRCMASKS_PORTS_MASK)
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + i,
+ VSC73XX_SRCMASKS_PORTS_MASK,
+ vsc->forward_map[i]);
+ }
+}
+
+static int vsc73xx_port_bridge_join(struct dsa_switch *ds, int port,
+ struct dsa_bridge bridge,
+ bool *tx_fwd_offload,
+ struct netlink_ext_ack *extack)
+{
+ struct vsc73xx *vsc = ds->priv;
+ int i;
+
+ *tx_fwd_offload = true;
+
+ for (i = 0; i < ds->num_ports; i++) {
+ /* Add this port to the forwarding matrix of the
+ * other ports in the same bridge, and viceversa.
+ */
+ if (!dsa_is_user_port(ds, i))
+ continue;
+
+ if (i == port)
+ continue;
+
+ if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+ continue;
+
+ vsc->forward_map[port] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(i);
+ vsc->forward_map[i] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(port);
+ }
+ vsc73xx_update_forwarding_map(vsc);
+
+ return dsa_tag_8021q_bridge_join(ds, port, bridge);
+}
+
+static void vsc73xx_port_bridge_leave(struct dsa_switch *ds, int port,
+ struct dsa_bridge bridge)
+{
+ struct vsc73xx *vsc = ds->priv;
+ int i;
+ /*configure forward map to CPU <-> port only*/
+ for (i = 0; i < vsc->ds->num_ports; i++) {
+ if (i == CPU_PORT)
+ continue;
+ vsc->forward_map[i] &= VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(port);
+ }
+ vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
+
+ vsc73xx_update_forwarding_map(vsc);
+ dsa_tag_8021q_bridge_leave(ds, port, bridge);
+}
+
static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
u16 flags)
{
@@ -1355,6 +1421,7 @@ static int vsc73xx_setup(struct dsa_switch *ds)
ds->vlan_filtering_is_global = false;
ds->configure_vlan_while_not_filtering = false;
+ ds->max_num_bridges = 7;
/* Issue RESET */
vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
@@ -1465,6 +1532,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.get_sset_count = vsc73xx_get_sset_count,
.port_enable = vsc73xx_port_enable,
.port_disable = vsc73xx_port_disable,
+ .port_bridge_join = vsc73xx_port_bridge_join,
+ .port_bridge_leave = vsc73xx_port_bridge_leave,
.port_change_mtu = vsc73xx_change_mtu,
.port_max_mtu = vsc73xx_get_max_mtu,
.port_stp_state_set = vsc73xx_port_stp_state_set,
--
2.34.1
On Wed, Jun 21, 2023 at 9:14 PM Pawel Dembicki <[email protected]> wrote:
> This patch implement vlan filtering for vsc73xx driver.
>
> After vlan filtering start, switch is reconfigured from QinQ to simple
> vlan aware mode. It's required, because VSC73XX chips haven't support
> for inner vlan tag filter.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Jun 21, 2023 at 9:14 PM Pawel Dembicki <[email protected]> wrote:
> This patch adds bridge support for vsc73xx driver.
> It introduce two functions for port_bridge_join and
> vsc73xx_port_bridge_leave handling.
>
> Those functions implement forwarding adjust and use
> dsa_tag_8021q_bridge_* api for adjust VLAN configuration.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Given that we use the approach from the other patches, this
makes perfect sense.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Jun 21, 2023 at 9:13 PM Pawel Dembicki <[email protected]> wrote:
> This patch replaces the adjust_link api with the phylink apis that provide
> equivalent functionality.
>
> The remaining functionality from the adjust_link is now covered in the
> phylink_mac_link_* and phylink_mac_config.
>
> Removes:
> .adjust_link
> Adds:
> .phylink_get_caps
> .phylink_mac_link_down
> .phylink_mac_link_up
> .phylink_mac_link_down
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Thanks for doing this!
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote:
> This patch replaces the adjust_link api with the phylink apis that provide
> equivalent functionality.
>
> The remaining functionality from the adjust_link is now covered in the
> phylink_mac_link_* and phylink_mac_config.
>
> Removes:
> .adjust_link
> Adds:
> .phylink_get_caps
> .phylink_mac_link_down
> .phylink_mac_link_up
> .phylink_mac_link_down
>
> Signed-off-by: Pawel Dembicki <[email protected]>
...
> +static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev,
> + int speed, int duplex,
> + bool tx_pause, bool rx_pause)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u32 val;
>
> + switch (speed) {
> + case SPEED_1000:
> /* Set up default for internal port or external RGMII */
> - if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
> + if (interface == PHY_INTERFACE_MODE_RGMII)
> val = VSC73XX_MAC_CFG_1000M_F_RGMII;
> else
> val = VSC73XX_MAC_CFG_1000M_F_PHY;
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else if (phydev->speed == SPEED_100) {
> - if (phydev->duplex == DUPLEX_FULL) {
> - val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 100 Mbit full duplex mode\n",
> - port);
> - } else {
> - val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 100 Mbit half duplex mode\n",
> - port);
> - }
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else if (phydev->speed == SPEED_10) {
> - if (phydev->duplex == DUPLEX_FULL) {
> + break;
> + case SPEED_100:
> + case SPEED_10:
> + if (duplex == DUPLEX_FULL)
> val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 10 Mbit full duplex mode\n",
> - port);
> - } else {
> + else
> val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 10 Mbit half duplex mode\n",
> - port);
> - }
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else {
> - dev_err(vsc->dev,
> - "could not adjust link: unknown speed\n");
> + break;
> }
>
> /* Enable port (forwarding) in the receieve mask */
> vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> VSC73XX_RECVMASK, BIT(port), BIT(port));
> + vsc73xx_adjust_enable_port(vsc, port, val);
Hi Pawel,
GCC 12.3.0 [1] reports that val may now be uninitialised at this point,
and in turn used uninitialised in vsc73xx_adjust_enable_port.
In function 'vsc73xx_adjust_enable_port',
inlined from 'vsc73xx_phylink_mac_link_up' at drivers/net/dsa/vitesse-vsc73xx-core.c:891:2:
drivers/net/dsa/vitesse-vsc73xx-core.c:725:13: warning: 'val' may be used uninitialized [-Wmaybe-uninitialized]
725 | val |= VSC73XX_MAC_CFG_RESET;
| ^
drivers/net/dsa/vitesse-vsc73xx-core.c: In function 'vsc73xx_phylink_mac_link_up':
drivers/net/dsa/vitesse-vsc73xx-core.c:869:13: note: 'val' was declared here
869 | u32 val;
| ^~~
...
On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote:
> + /* This driver does not make use of the speed, duplex, pause or the
> + * advertisement in its mac_config, so it is safe to mark this driver
> + * as non-legacy.
> + */
> + config->legacy_pre_march2020 = false;
Great stuff, thanks!
> +static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> + struct vsc73xx *vsc = ds->priv;
Nit: normally have a blank line between function variable declarations
and the rest of the function body.
> /* Special handling of the CPU-facing port */
> if (port == CPU_PORT) {
> /* Other ports are already initialized but not this one */
> @@ -775,104 +803,92 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
> VSC73XX_ADVPORTM_ENA_GTX |
> VSC73XX_ADVPORTM_DDR_MODE);
> }
> +}
>
> - /* This is the MAC confiuration that always need to happen
> - * after a PHY or the CPU port comes up or down.
> - */
> - if (!phydev->link) {
> - int maxloop = 10;
> +static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u32 val;
>
> - dev_dbg(vsc->dev, "port %d: went down\n",
> - port);
> + int maxloop = VSC73XX_TABLE_ATTEMPTS;
Reverse christmas-tree for variable declarations, and there shouldn't
be a blank line between them. In any case, I don't think you need
"maxloop" if you adopt my suggestion below.
>
> - /* Disable RX on this port */
> - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> - VSC73XX_MAC_CFG,
> - VSC73XX_MAC_CFG_RX_EN, 0);
> + dev_dbg(vsc->dev, "port %d: went down\n",
> + port);
>
> - /* Discard packets */
> - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> - VSC73XX_ARBDISC, BIT(port), BIT(port));
> + /* Disable RX on this port */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_MAC_CFG,
> + VSC73XX_MAC_CFG_RX_EN, 0);
> +
> + /* Discard packets */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> + VSC73XX_ARBDISC, BIT(port), BIT(port));
>
> - /* Wait until queue is empty */
> + /* Wait until queue is empty */
> + vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> + VSC73XX_ARBEMPTY, &val);
> + while (!(val & BIT(port))) {
> + msleep(1);
> vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> VSC73XX_ARBEMPTY, &val);
> - while (!(val & BIT(port))) {
> - msleep(1);
> - vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> - VSC73XX_ARBEMPTY, &val);
> - if (--maxloop == 0) {
> - dev_err(vsc->dev,
> - "timeout waiting for block arbiter\n");
> - /* Continue anyway */
> - break;
> - }
> + if (--maxloop == 0) {
> + dev_err(vsc->dev,
> + "timeout waiting for block arbiter\n");
> + /* Continue anyway */
> + break;
> }
> + }
I know you're only moving this code, but I think it would be good to
_first_ have a patch that fixes this polling function:
int ret, err;
...
ret = read_poll_timeout(vsc73xx_read, err,
err < 0 || (val & BIT(port)),
1000, 10000, false,
vsc, VSC73XX_BLOCK_ARBITER, 0,
VSC73XX_ARBEMPTY, &val);
if (ret != 0)
dev_err(vsc->dev,
"timeout waiting for block arbiter\n");
else if (err < 0)
dev_err(vsc->dev,
"error reading arbiter\n");
This avoids the issue that on the last iteration, the code reads the
register, test it, find the condition that's being waiting for is
false, _then_ waits and end up printing the error message - that last
wait is rather useless, and as the arbiter state isn't checked after
waiting, it could be that we had success during the last wait.
> +static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev,
> + int speed, int duplex,
> + bool tx_pause, bool rx_pause)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u32 val;
>
> + switch (speed) {
> + case SPEED_1000:
> /* Set up default for internal port or external RGMII */
> - if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
> + if (interface == PHY_INTERFACE_MODE_RGMII)
> val = VSC73XX_MAC_CFG_1000M_F_RGMII;
> else
> val = VSC73XX_MAC_CFG_1000M_F_PHY;
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else if (phydev->speed == SPEED_100) {
> - if (phydev->duplex == DUPLEX_FULL) {
> - val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 100 Mbit full duplex mode\n",
> - port);
> - } else {
> - val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 100 Mbit half duplex mode\n",
> - port);
> - }
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else if (phydev->speed == SPEED_10) {
> - if (phydev->duplex == DUPLEX_FULL) {
> + break;
> + case SPEED_100:
> + case SPEED_10:
> + if (duplex == DUPLEX_FULL)
> val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 10 Mbit full duplex mode\n",
> - port);
> - } else {
> + else
> val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 10 Mbit half duplex mode\n",
> - port);
> - }
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else {
> - dev_err(vsc->dev,
> - "could not adjust link: unknown speed\n");
> + break;
> }
Do the dev_dbg() add anything useful over what phylink prints when the
link comes up?
I don't think moving to a switch() statement for this is a good idea.
Given that "val" may be uninitialised, I suspect the following may be
a better solution:
if (speed == SPEED_1000 || speed == SPEED_100 || speed == SPEED_10) {
if (speed == SPEED_1000) {
...
} else {
...
}
... set VSC73XX_BLOCK_ANALYZER and call
vsc73xx_adjust_enable_port ...
}
However, looking at the definitions of the various macros, it seems we
can do a little better by not using the VSC73XX_MAC_CFG_*M_[FH]_*
definitions:
if (speed == SPEED_1000) {
val = VSC73XX_MAC_CFG_GIGA_MODE |
VSC73XX_MAC_CFG_TX_IPG_1000M;
if (interface == PHY_INTERFACE_MODE_RGMII)
val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
else
val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
} else {
val = VSC73XX_MAC_CFG_TX_IPG_100_10M |
VSC73XX_MAC_CFG_CLK_SEL_EXT;
}
if (duplex == DUPLEX_FULL)
val |= VSC73XX_MAC_CFG_FDX;
Now, this reveals a question: when operating in RGMII mode, why do we
need VSC73XX_MAC_CFG_CLK_SEL_1000M for 1G, and
VSC73XX_MAC_CFG_CLK_SEL_EXT for 10M and 100M, whereas "PHY" mode always
uses CLK_SEL_EXT ?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
czw., 22 cze 2023 o 13:55 Russell King (Oracle)
<[email protected]> napisał(a):
>
> On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote:
> > + /* This driver does not make use of the speed, duplex, pause or the
> > + * advertisement in its mac_config, so it is safe to mark this driver
> > + * as non-legacy.
> > + */
> > + config->legacy_pre_march2020 = false;
>
> Great stuff, thanks!
>
> > +static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + const struct phylink_link_state *state)
> > +{
> > + struct vsc73xx *vsc = ds->priv;
>
> Nit: normally have a blank line between function variable declarations
> and the rest of the function body.
>
> > /* Special handling of the CPU-facing port */
> > if (port == CPU_PORT) {
> > /* Other ports are already initialized but not this one */
> > @@ -775,104 +803,92 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
> > VSC73XX_ADVPORTM_ENA_GTX |
> > VSC73XX_ADVPORTM_DDR_MODE);
> > }
> > +}
> >
> > - /* This is the MAC confiuration that always need to happen
> > - * after a PHY or the CPU port comes up or down.
> > - */
> > - if (!phydev->link) {
> > - int maxloop = 10;
> > +static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + phy_interface_t interface)
> > +{
> > + struct vsc73xx *vsc = ds->priv;
> > + u32 val;
> >
> > - dev_dbg(vsc->dev, "port %d: went down\n",
> > - port);
> > + int maxloop = VSC73XX_TABLE_ATTEMPTS;
>
> Reverse christmas-tree for variable declarations, and there shouldn't
> be a blank line between them. In any case, I don't think you need
> "maxloop" if you adopt my suggestion below.
>
> >
> > - /* Disable RX on this port */
> > - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > - VSC73XX_MAC_CFG,
> > - VSC73XX_MAC_CFG_RX_EN, 0);
> > + dev_dbg(vsc->dev, "port %d: went down\n",
> > + port);
> >
> > - /* Discard packets */
> > - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> > - VSC73XX_ARBDISC, BIT(port), BIT(port));
> > + /* Disable RX on this port */
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_MAC_CFG,
> > + VSC73XX_MAC_CFG_RX_EN, 0);
> > +
> > + /* Discard packets */
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> > + VSC73XX_ARBDISC, BIT(port), BIT(port));
> >
> > - /* Wait until queue is empty */
> > + /* Wait until queue is empty */
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> > + VSC73XX_ARBEMPTY, &val);
> > + while (!(val & BIT(port))) {
> > + msleep(1);
> > vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> > VSC73XX_ARBEMPTY, &val);
> > - while (!(val & BIT(port))) {
> > - msleep(1);
> > - vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> > - VSC73XX_ARBEMPTY, &val);
> > - if (--maxloop == 0) {
> > - dev_err(vsc->dev,
> > - "timeout waiting for block arbiter\n");
> > - /* Continue anyway */
> > - break;
> > - }
> > + if (--maxloop == 0) {
> > + dev_err(vsc->dev,
> > + "timeout waiting for block arbiter\n");
> > + /* Continue anyway */
> > + break;
> > }
> > + }
>
> I know you're only moving this code, but I think it would be good to
> _first_ have a patch that fixes this polling function:
>
> int ret, err;
> ...
> ret = read_poll_timeout(vsc73xx_read, err,
> err < 0 || (val & BIT(port)),
> 1000, 10000, false,
> vsc, VSC73XX_BLOCK_ARBITER, 0,
> VSC73XX_ARBEMPTY, &val);
> if (ret != 0)
> dev_err(vsc->dev,
> "timeout waiting for block arbiter\n");
> else if (err < 0)
> dev_err(vsc->dev,
> "error reading arbiter\n");
>
> This avoids the issue that on the last iteration, the code reads the
> register, test it, find the condition that's being waiting for is
> false, _then_ waits and end up printing the error message - that last
> wait is rather useless, and as the arbiter state isn't checked after
> waiting, it could be that we had success during the last wait.
>
Thank you for the tips. I will prepare additional commit in v2 series.
> > +static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + phy_interface_t interface,
> > + struct phy_device *phydev,
> > + int speed, int duplex,
> > + bool tx_pause, bool rx_pause)
> > +{
> > + struct vsc73xx *vsc = ds->priv;
> > + u32 val;
> >
> > + switch (speed) {
> > + case SPEED_1000:
> > /* Set up default for internal port or external RGMII */
> > - if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
> > + if (interface == PHY_INTERFACE_MODE_RGMII)
> > val = VSC73XX_MAC_CFG_1000M_F_RGMII;
> > else
> > val = VSC73XX_MAC_CFG_1000M_F_PHY;
> > - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> > - } else if (phydev->speed == SPEED_100) {
> > - if (phydev->duplex == DUPLEX_FULL) {
> > - val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> > - dev_dbg(vsc->dev,
> > - "port %d: 100 Mbit full duplex mode\n",
> > - port);
> > - } else {
> > - val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> > - dev_dbg(vsc->dev,
> > - "port %d: 100 Mbit half duplex mode\n",
> > - port);
> > - }
> > - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> > - } else if (phydev->speed == SPEED_10) {
> > - if (phydev->duplex == DUPLEX_FULL) {
> > + break;
> > + case SPEED_100:
> > + case SPEED_10:
> > + if (duplex == DUPLEX_FULL)
> > val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> > - dev_dbg(vsc->dev,
> > - "port %d: 10 Mbit full duplex mode\n",
> > - port);
> > - } else {
> > + else
> > val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> > - dev_dbg(vsc->dev,
> > - "port %d: 10 Mbit half duplex mode\n",
> > - port);
> > - }
> > - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> > - } else {
> > - dev_err(vsc->dev,
> > - "could not adjust link: unknown speed\n");
> > + break;
> > }
>
> Do the dev_dbg() add anything useful over what phylink prints when the
> link comes up?
>
> I don't think moving to a switch() statement for this is a good idea.
> Given that "val" may be uninitialised, I suspect the following may be
> a better solution:
>
> if (speed == SPEED_1000 || speed == SPEED_100 || speed == SPEED_10) {
> if (speed == SPEED_1000) {
> ...
> } else {
> ...
> }
>
> ... set VSC73XX_BLOCK_ANALYZER and call
> vsc73xx_adjust_enable_port ...
> }
>
> However, looking at the definitions of the various macros, it seems we
> can do a little better by not using the VSC73XX_MAC_CFG_*M_[FH]_*
> definitions:
>
> if (speed == SPEED_1000) {
> val = VSC73XX_MAC_CFG_GIGA_MODE |
> VSC73XX_MAC_CFG_TX_IPG_1000M;
>
> if (interface == PHY_INTERFACE_MODE_RGMII)
> val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
> else
> val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
> } else {
> val = VSC73XX_MAC_CFG_TX_IPG_100_10M |
> VSC73XX_MAC_CFG_CLK_SEL_EXT;
> }
>
> if (duplex == DUPLEX_FULL)
> val |= VSC73XX_MAC_CFG_FDX;
>
> Now, this reveals a question: when operating in RGMII mode, why do we
> need VSC73XX_MAC_CFG_CLK_SEL_1000M for 1G, and
> VSC73XX_MAC_CFG_CLK_SEL_EXT for 10M and 100M, whereas "PHY" mode always
> uses CLK_SEL_EXT ?
>
VSC73XX_MAC_CFG_CLK_SEL_EXT should be used always when phy is used, no
matter what speed is. VSC73XX_MAC_CFG_CLK_SEL_1000M in RGMII mode. It
can be even more simplified:
if (speed == SPEED_1000)
val = VSC73XX_MAC_CFG_GIGA_MODE | VSC73XX_MAC_CFG_TX_IPG_1000M;
else
val = VSC73XX_MAC_CFG_TX_IPG_100_10M;
if (interface == PHY_INTERFACE_MODE_RGMII)
val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
else
val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
if (duplex == DUPLEX_FULL)
val |= VSC73XX_MAC_CFG_FDX;
--
Pawel Dembicki