lkml.org 
[lkml]   [2023]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2023-03-29 00:46    [W:2.386 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site