lkml.org 
[lkml]   [2017]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods
    Date
    Hi Egil,

    Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

    > Add DSA method port_fast_age as a step to STP support.
    >
    > Add low level functions for accessing the lan9303 ALR (Address Logic
    > Resolution).
    >
    > Added DSA method port_fdb_dump
    >
    > Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

    The patch looks good overall, a few nitpicks though.

    > ---
    > drivers/net/dsa/lan9303-core.c | 159 +++++++++++++++++++++++++++++++++++++++++
    > drivers/net/dsa/lan9303.h | 2 +
    > 2 files changed, 161 insertions(+)
    >
    > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
    > index 09a748327fc6..ae904242b001 100644
    > --- a/drivers/net/dsa/lan9303-core.c
    > +++ b/drivers/net/dsa/lan9303-core.c
    > @@ -124,6 +124,21 @@
    > #define LAN9303_MAC_RX_CFG_2 0x0c01
    > #define LAN9303_MAC_TX_CFG_2 0x0c40
    > #define LAN9303_SWE_ALR_CMD 0x1800
    > +# define ALR_CMD_MAKE_ENTRY BIT(2)
    > +# define ALR_CMD_GET_FIRST BIT(1)
    > +# define ALR_CMD_GET_NEXT BIT(0)
    > +#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
    > +#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
    > +# define ALR_DAT1_VALID BIT(26)
    > +# define ALR_DAT1_END_OF_TABL BIT(25)
    > +# define ALR_DAT1_AGE_OVERRID BIT(25)
    > +# define ALR_DAT1_STATIC BIT(24)
    > +# define ALR_DAT1_PORT_BITOFFS 16
    > +# define ALR_DAT1_PORT_MASK (7 << ALR_DAT1_PORT_BITOFFS)
    > +#define LAN9303_SWE_ALR_RD_DAT_0 0x1805
    > +#define LAN9303_SWE_ALR_RD_DAT_1 0x1806
    > +#define LAN9303_SWE_ALR_CMD_STS 0x1808
    > +# define ALR_STS_MAKE_PEND BIT(0)

    Why is there different spacing and prefix with these defines?

    > #define LAN9303_SWE_VLAN_CMD 0x180b
    > # define LAN9303_SWE_VLAN_CMD_RNW BIT(5)
    > # define LAN9303_SWE_VLAN_CMD_PVIDNVLAN BIT(4)
    > @@ -478,6 +493,125 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
    > return 0;
    > }
    >
    > +/* ----------------- Address Logic Resolution (ALR)------------------*/
    > +
    > +/* Map ALR-port bits to port bitmap, and back*/

    The (leading and trailing) spacing in your comments is often
    inconsistent. You can use simple inline or block comments, no need for
    fancy rules. Please refer to Documentation/networking/netdev-FAQ.txt for
    block comment style in netdev though, they are a bit different.

    > +static const int alrport_2_portmap[] = {1, 2, 4, 0, 3, 5, 6, 7 };
    > +static const int portmap_2_alrport[] = {3, 0, 1, 4, 2, 5, 6, 7 };
    > +
    > +/* ALR: Actual register access functions */
    > +
    > +/* This function will wait a while until mask & reg == value */
    > +/* Otherwise, return timeout */

    Same, a single block comment will do the job.

    > +static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
    > + int mask, char value)
    > +{
    > + int i;
    > +
    > + for (i = 0; i < 0x1000; i++) {
    > + u32 reg;
    > +
    > + lan9303_read_switch_reg(chip, regno, &reg);
    > + if ((reg & mask) == value)
    > + return 0;
    > + usleep_range(1000, 2000);
    > + }
    > + return -ETIMEDOUT;
    > +}
    > +
    > +static int lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1)
    > +{
    > + lan9303_write_switch_reg(
    > + chip, LAN9303_SWE_ALR_WR_DAT_0, dat0);
    > + lan9303_write_switch_reg(
    > + chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
    > + lan9303_write_switch_reg(
    > + chip, LAN9303_SWE_ALR_CMD, ALR_CMD_MAKE_ENTRY);
    > + lan9303_csr_reg_wait(
    > + chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND, 0);

    As I said in a previous series, please don't do this. Function arguments
    must be vertically aligned with the opening parenthesis.

    > + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
    > + return 0;

    A newline before a return statement is appreciated.

    > +}
    > +
    > +typedef void alr_loop_cb_t(struct lan9303 *chip, u32 dat0, u32 dat1,
    > + int portmap, void *ctx);
    > +
    > +static void lan9303_alr_loop(struct lan9303 *chip, alr_loop_cb_t *cb, void *ctx)
    > +{
    > + int i;
    > +
    > + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_FIRST);
    > + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
    > +
    > + for (i = 1; i < LAN9303_NUM_ALR_RECORDS; i++) {
    > + u32 dat0, dat1;
    > + int alrport, portmap;
    > +
    > + lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_0, &dat0);
    > + lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, &dat1);
    > + if (dat1 & ALR_DAT1_END_OF_TABL)
    > + break;
    > +
    > + alrport = (dat1 & ALR_DAT1_PORT_MASK) >> ALR_DAT1_PORT_BITOFFS;
    > + portmap = alrport_2_portmap[alrport];
    > +
    > + cb(chip, dat0, dat1, portmap, ctx);
    > +
    > + lan9303_write_switch_reg(
    > + chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_NEXT);

    Please align arguments with the opening parenthesis.

    > + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
    > + }
    > +}
    > +
    > +/* ALR: lan9303_alr_loop callback functions */
    > +

    No need for an extra newline if your comment refers directly to the
    function below. It will also be consistent with the rest of your patch.

    > +static void alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6])
    > +{
    > + mac[0] = (dat0 >> 0) & 0xff;
    > + mac[1] = (dat0 >> 8) & 0xff;
    > + mac[2] = (dat0 >> 16) & 0xff;
    > + mac[3] = (dat0 >> 24) & 0xff;
    > + mac[4] = (dat1 >> 0) & 0xff;
    > + mac[5] = (dat1 >> 8) & 0xff;
    > +}
    > +
    > +/* Clear learned (non-static) entry on given port */
    > +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
    > + u32 dat1, int portmap, void *ctx)
    > +{
    > + int *port = ctx;

    You can get the value directly to make the line below more readable:

    int port = *(int *)ctx;

    > +
    > + if (((BIT(*port) & portmap) == 0) || (dat1 & ALR_DAT1_STATIC))
    > + return;
    > +
    > + /* learned entries has only one port, we can just delete */
    > + dat1 &= ~ALR_DAT1_VALID; /* delete entry */
    > + lan9303_alr_make_entry_raw(chip, dat0, dat1);
    > +}
    > +
    > +struct port_fdb_dump_ctx {
    > + int port;
    > + void *data;
    > + dsa_fdb_dump_cb_t *cb;
    > +};
    > +
    > +static void alr_loop_cb_fdb_port_dump(struct lan9303 *chip, u32 dat0,
    > + u32 dat1, int portmap, void *ctx)
    > +{
    > + struct port_fdb_dump_ctx *dump_ctx = ctx;
    > + u8 mac[ETH_ALEN];
    > + bool is_static;
    > +
    > + if ((BIT(dump_ctx->port) & portmap) == 0)
    > + return;
    > +
    > + alr_reg_to_mac(dat0, dat1, mac);
    > + is_static = !!(dat1 & ALR_DAT1_STATIC);
    > + dump_ctx->cb(mac, 0, is_static, dump_ctx->data);
    > +}
    > +
    > +/* --------------------- Various chip setup ----------------------*/
    > +

    This isn't a very useful comment, at least use an inline or block
    comment if you want to keep it.

    > static int lan9303_disable_processing_port(struct lan9303 *chip,
    > unsigned int port)
    > {
    > @@ -923,6 +1057,29 @@ static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
    > /* else: touching SWE_PORT_STATE would break port separation */
    > }
    >
    > +static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
    > +{
    > + struct lan9303 *chip = ds->priv;
    > +
    > + dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
    > + lan9303_alr_loop(chip, alr_loop_cb_del_port_learned, &port);
    > +}
    > +
    > +static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port,
    > + dsa_fdb_dump_cb_t *cb, void *data)
    > +{
    > + struct lan9303 *chip = ds->priv;
    > + struct port_fdb_dump_ctx dump_ctx = {
    > + .port = port,
    > + .data = data,
    > + .cb = cb,
    > + };
    > +
    > + dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
    > + lan9303_alr_loop(chip, alr_loop_cb_fdb_port_dump, &dump_ctx);
    > + return 0;

    A newline before the return statement is welcome.

    > +}
    > +
    > static const struct dsa_switch_ops lan9303_switch_ops = {
    > .get_tag_protocol = lan9303_get_tag_protocol,
    > .setup = lan9303_setup,
    > @@ -937,6 +1094,8 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
    > .port_bridge_join = lan9303_port_bridge_join,
    > .port_bridge_leave = lan9303_port_bridge_leave,
    > .port_stp_state_set = lan9303_port_stp_state_set,
    > + .port_fast_age = lan9303_port_fast_age,
    > + .port_fdb_dump = lan9303_port_fdb_dump,
    > };
    >
    > static int lan9303_register_switch(struct lan9303 *chip)
    > diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
    > index 68ecd544b658..4db323d65741 100644
    > --- a/drivers/net/dsa/lan9303.h
    > +++ b/drivers/net/dsa/lan9303.h
    > @@ -11,6 +11,8 @@ struct lan9303_phy_ops {
    > int regnum, u16 val);
    > };
    >
    > +#define LAN9303_NUM_ALR_RECORDS 512
    > +
    > struct lan9303 {
    > struct device *dev;
    > struct regmap *regmap;
    > --
    > 2.11.0

    \
     
     \ /
      Last update: 2017-10-22 17:24    [W:6.931 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site