Messages in this thread Patch in this message | | | Date | Wed, 29 Mar 2023 00:45:37 +0200 | From | Andrew Lunn <> | Subject | Re: [RFC PATCH net-next v2 2/2] net: dsa: mt7530: introduce MMIO driver for MT7988 SoC |
| |
> @@ -146,11 +149,13 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val) > { > struct mii_bus *bus = priv->bus; > > - mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > + if (bus) > + mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > > core_write_mmd_indirect(priv, reg, MDIO_MMD_VEND2, val); > > - mutex_unlock(&bus->mdio_lock); > + if (bus) > + mutex_unlock(&bus->mdio_lock); > }
All this if (bus) is pretty ugly.
First off, what is this mutex actually protecting? And why is the same protection not required for MMIO?
If you have a convincing argument the mutex is not needed, please add two helpers:
mt7530_mutex_lock(struct mt7530_priv *priv) mt7530_mutex_unlock(struct mt7530_priv *priv)
and hide the if inside that. As part of the commit message, explain why the mutex is not needed for MDIO.
Adding these helpers and changing all calls can be a preparation patch. It is the sort of patch which should be obviously correct.
> @@ -2588,6 +2664,8 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode, > case PHY_INTERFACE_MODE_NA: > case PHY_INTERFACE_MODE_1000BASEX: > case PHY_INTERFACE_MODE_2500BASEX: > + case PHY_INTERFACE_MODE_USXGMII: > + case PHY_INTERFACE_MODE_10GKR: > /* handled in SGMII PCS driver */ > return 0; > default: > @@ -2712,7 +2790,9 @@ static void mt753x_phylink_mac_link_up(struct dsa_switch *ds, int port, > * variants. > */ > if (interface == PHY_INTERFACE_MODE_TRGMII || > - (phy_interface_mode_is_8023z(interface))) { > + interface == PHY_INTERFACE_MODE_USXGMII || > + interface == PHY_INTERFACE_MODE_10GKR || > + phy_interface_mode_is_8023z(interface)) { > speed = SPEED_1000; > duplex = DUPLEX_FULL; > }
This looks like something which should be in a separate patch.
> +static int mt7988_setup(struct dsa_switch *ds) > +{ > + struct mt7530_priv *priv = ds->priv; > + u32 unused_pm = 0; > + int i; > + > + /* Reset the switch */ > + reset_control_assert(priv->rstc); > + udelay(20); > + reset_control_deassert(priv->rstc); > + udelay(20); > + > + /* Reset the switch PHYs */ > + mt7530_write(priv, MT7530_SYS_CTRL, SYS_CTRL_PHY_RST); > + > + /* BPDU to CPU port */ > + mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK, > + BIT(MT7988_CPU_PORT)); > + mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK, > + MT753X_BPDU_CPU_ONLY); > + > + /* Enable and reset MIB counters */ > + mt7530_mib_reset(ds); > + > + for (i = 0; i < MT7530_NUM_PORTS; i++) { > + /* Disable forwarding by default on all ports */ > + mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK, > + PCR_MATRIX_CLR); > + > + mt7530_set(priv, MT7531_DBG_CNT(i), MT7531_DIS_CLR); > + > + if (dsa_is_unused_port(ds, i)) > + unused_pm |= BIT(i); > + else if (dsa_is_cpu_port(ds, i)) > + mt753x_cpu_port_enable(ds, i); > + else > + mt7530_port_disable(ds, i); > + > + /* Enable consistent egress tag */ > + mt7530_rmw(priv, MT7530_PVC_P(i), PVC_EG_TAG_MASK, > + PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT)); > + } > + > + ds->configure_vlan_while_not_filtering = true; > + > + /* Flush the FDB table */ > + return mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
Is this really specific to the mt7988?
Andrew
| |