lkml.org 
[lkml]   [2016]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2 3/3] net-next: dsa: add new driver for qca8xxx family
    On Wed, Sep 14, 2016 at 12:39:02PM +0200, John Crispin wrote:
    > This patch contains initial support for the QCA8337 switch. It
    > will detect a QCA8337 switch, if present and declared in the DT.
    >
    > Each port will be represented through a standalone net_device interface,
    > as for other DSA switches. CPU can communicate with any of the ports by
    > setting an IP@ on ethN interface. Most of the extra callbacks of the DSA
    > subsystem are already supported, such as bridge offloading, stp, fdb.
    >
    > Signed-off-by: John Crispin <john@phrozen.org>
    > ---
    > Changes in V2
    > * add proper locking for the FDB table
    > * remove udelay when changing the page. neither datasheet nore SDK code
    > requires a sleep
    > * add a cond_resched to the busy wait loop
    > * use nested locking when accessing the mdio bus
    > * remove the phy_to_port() wrappers
    > * remove mmd access function and use existing phy helpers
    > * fix a copy/paste bug breaking the eee callbacks
    > * use default vid 1 when fdb entries are added fro vid 0
    > * remove the phy id check and add a switch id check instead
    > * add error handling to the mdio read/write functions
    > * remove inline usage

    Hi John

    Thanks for the changes, it looks a lot better.

    > +static u16 qca8k_current_page = 0xffff;
    > +
    > +static struct
    > +qca8k_priv *qca8k_to_priv(struct dsa_switch *ds)
    > +{
    > + struct qca8k_priv *priv = ds->priv;
    > +
    > + return priv;
    > +}

    https://lkml.org/lkml/2016/8/31/936

    In this patch, Vivien removed all the callers to ds_to_priv(). Which
    is what this function is. Please just use ds->priv.

    > +
    > +static void
    > +qca8k_fdb_flush(struct qca8k_priv *priv)
    > +{
    > + mutex_lock(&priv->fdb_mutex);
    > + qca8k_fdb_access(priv, QCA8K_FDB_FLUSH, -1);
    > + mutex_unlock(&priv->fdb_mutex);
    > +}

    So this protects the fdb. But should this mutex be more general. Take for example:

    > +qca8k_eee_enable_set(struct dsa_switch *ds, int port, bool enable)
    > +{
    > + struct qca8k_priv *priv = qca8k_to_priv(ds);
    > + u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port);
    > + u32 reg;
    > +
    > + reg = qca8k_read(priv, QCA8K_REG_EEE_CTRL);
    > + if (enable)
    > + reg |= lpi_en;
    > + else
    > + reg &= ~lpi_en;
    > + qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
    > +}
    > +

    Here you do a read/modify/write. Could this be called on two
    interfaces at the same time? I think you might want this protected as
    well by a mutex. So maybe rename fdb_mutex to something more generic
    and use it in places like this as well?

    > +static int
    > +qca8k_setup(struct dsa_switch *ds)
    > +{
    > + struct qca8k_priv *priv = qca8k_to_priv(ds);
    > + int ret, i, phy_mode = -1;
    > +
    > + /* Start by setting up the register mapping */
    > + priv->regmap = devm_regmap_init(ds->dev, NULL, priv,
    > + &qca8k_regmap_config);
    > +
    > + if (IS_ERR(priv->regmap))
    > + pr_warn("regmap initialization failed");
    > +
    > + /* Initialize CPU port pad mode (xMII type, delays...) */
    > + phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
    > + if (phy_mode < 0) {
    > + pr_err("Can't find phy-mode for master device\n");
    > + return phy_mode;
    > + }
    > + ret = qca8k_set_pad_ctrl(priv, QCA8K_CPU_PORT, phy_mode);
    > + if (ret < 0)
    > + return ret;

    Maybe add a check here that dsa_is_cpu_port(ds, 0) is true? If you
    say the CPU port has to be 0, it should be checked for.

    > + /* Enable CPU Port */
    > + qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
    > + QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
    > +
    > + /* Enable MIB counters */
    > + qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
    > + qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);

    Will this implicitly clear the MIB counters? They should be set to
    zero, otherwise they can survive a reboot of the host.

    > +
    > + /* Setup connection between CPU ports & PHYs */

    You missed a few "PHY". We tend to call such ports user ports, or
    external ports.

    > + for (i = 0; i < DSA_MAX_PORTS; i++) {
    > + /* CPU port gets connected to all PHYs in the switch */
    > + if (dsa_is_cpu_port(ds, i)) {
    > + qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(QCA8K_CPU_PORT),
    > + QCA8K_PORT_LOOKUP_MEMBER,
    > + ds->enabled_port_mask);
    > + }
    > +
    > + /* Invividual PHYs gets connected to CPU port only */
    > + if (ds->enabled_port_mask & BIT(i)) {
    > + int shift = 16 * (i % 2);
    > +
    > + qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
    > + QCA8K_PORT_LOOKUP_MEMBER,
    > + BIT(QCA8K_CPU_PORT));
    > +
    > + /* Enable ARP Auto-learning by default */
    > + qca8k_reg_set(priv, QCA8K_PORT_LOOKUP_CTRL(i),
    > + QCA8K_PORT_LOOKUP_LEARN);
    > +
    > + /* For port based vlans to work we need to set the
    > + * default egress vid
    > + */
    > + qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
    > + 0xffff << shift, 1 << shift);
    > + qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
    > + QCA8K_PORT_VLAN_CVID(1) |
    > + QCA8K_PORT_VLAN_SVID(1));
    > + }
    > + }
    > +
    > + /* Flush the FDB table */
    > + qca8k_fdb_flush(priv);
    > +
    > + return 0;
    > +}
    > +
    > +static int
    > +qca8k_set_addr(struct dsa_switch *ds, u8 *addr)
    > +{
    > + /* The subsystem always calls this function so add an empty stub */
    > + return 0;
    > +}

    The b53/bcm_sf2 driver also has a NOP for set_addr(). Maybe it is time
    to make it optional. But that is a separate patchset. This is O.K.

    > +static void
    > +qca8k_get_strings(struct dsa_switch *ds, int phy, uint8_t *data)

    In general, please can you not use int phy, when the prototype is int
    port. This is a bad example, since phy/port is not actually used,
    but... In fact, it does not happen so often, so maybe you just missed
    this one?

    > +static int
    > +qca8k_fdb_prepare(struct dsa_switch *ds, int port,
    > + const struct switchdev_obj_port_fdb *fdb,
    > + struct switchdev_trans *trans)
    > +{
    > + /* We do not need to do anything specific here yet */
    > + return 0;
    > +}

    Does this mean you can store an infinite number of fdb entries?

    > +static void
    > +qca8k_sw_remove(struct mdio_device *mdiodev)
    > +{
    > + struct qca8k_priv *priv = dev_get_drvdata(&mdiodev->dev);
    > +
    > + dsa_unregister_switch(priv->ds);
    > +}

    This has the same problem as the mv88e6xxx driver. You should disable
    all the ports when removing the driver. Something on my TODO list...

    > +
    > +#ifdef CONFIG_PM_SLEEP
    > +static int qca8k_suspend(struct device *dev)
    > +{
    > + struct platform_device *pdev = to_platform_device(dev);
    > + struct qca8k_priv *priv = platform_get_drvdata(pdev);
    > +
    > + return dsa_switch_suspend(priv->ds);
    > +}
    > +
    > +static int qca8k_resume(struct device *dev)
    > +{
    > + struct platform_device *pdev = to_platform_device(dev);
    > + struct qca8k_priv *priv = platform_get_drvdata(pdev);
    > +
    > + return dsa_switch_resume(priv->ds);
    > +}
    > +#endif /* CONFIG_PM_SLEEP */

    The bcm_sf2 driver disables the port on suspend, and re-enables them
    on resume. You should probably do the same.

    Thanks

    Andrew

    \
     
     \ /
      Last update: 2016-09-17 09:59    [W:2.371 / U:0.808 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site