IronPort-SDR: 9j6vy0oJdhtisLYaaSDLtN4+C7TQKOk7JqZD0UC8NlQfC0FqZlb80ARpx+XpR+QeeWq7W5TJpm
 YhA49sxwljhZo2isU+5wsV9yG0MGmqmbODXAg8OHJv1wkHGFV6FtSvMFhy1QPrugRfZOPcuTJa
 whdHlYzwlOCR+p+CdApHz+8Kt0uY89GDS6frmCZIoHo0Jzk5zZyClgdlNsRSs2SYeHD18YkA7M
 w9QhGzlwmOkGqBSnO6gfh+HdaAWN/+q2gmYQHetEg8ev4lntKQ5GWD2yPG3A0dEpxoVCRAN9po
 6XNQ/sGpADLRGCl8OlxS3nTq
Date:   Tue, 4 Jan 2022 14:40:57 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
        "vivien.didelot@gmail.com" <vivien.didelot@gmail.com>,
        "andrew@lunn.ch" <andrew@lunn.ch>
Subject: Re: [PATCH net-next v2 1/3] net: lan966x: Add function
 lan966x_mac_ip_learn()
Message-ID: <20220104134057.wj3wxba2glnz5k74@soft-dev3-1.localhost>
References: <20220104101849.229195-1-horatiu.vultur@microchip.com>
 <20220104101849.229195-2-horatiu.vultur@microchip.com>
 <20220104111710.twaqos2fbmjfv5yu@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20220104111710.twaqos2fbmjfv5yu@skbuf>
Precedence: bulk

The 01/04/2022 11:17, Vladimir Oltean wrote:
> 
> On Tue, Jan 04, 2022 at 11:18:47AM +0100, Horatiu Vultur wrote:
> > Extend mac functionality with the function lan966x_mac_ip_learn. This
> > function adds an entry in the MAC table for IP multicast addresses.
> > These entries can copy a frame to the CPU but also can forward on the
> > front ports.
> > This functionality is needed for mdb support. In case the CPU and some
> > of the front ports subscribe to an IP multicast address.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../ethernet/microchip/lan966x/lan966x_mac.c  | 33 ++++++++++++++++---
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  5 +++
> >  .../ethernet/microchip/lan966x/lan966x_regs.h |  6 ++++
> >  3 files changed, 39 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> > index efadb8d326cc..82eb6606e17f 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> > @@ -68,17 +68,19 @@ static void lan966x_mac_select(struct lan966x *lan966x,
> >       lan_wr(mach, lan966x, ANA_MACHDATA);
> >  }
> >
> > -int lan966x_mac_learn(struct lan966x *lan966x, int port,
> > -                   const unsigned char mac[ETH_ALEN],
> > -                   unsigned int vid,
> > -                   enum macaccess_entry_type type)
> > +static int __lan966x_mac_learn(struct lan966x *lan966x, int pgid,
> > +                            bool cpu_copy,
> > +                            const unsigned char mac[ETH_ALEN],
> > +                            unsigned int vid,
> > +                            enum macaccess_entry_type type)
> >  {
> >       lan966x_mac_select(lan966x, mac, vid);
> >
> >       /* Issue a write command */
> >       lan_wr(ANA_MACACCESS_VALID_SET(1) |
> >              ANA_MACACCESS_CHANGE2SW_SET(0) |
> > -            ANA_MACACCESS_DEST_IDX_SET(port) |
> > +            ANA_MACACCESS_MAC_CPU_COPY_SET(cpu_copy) |
> > +            ANA_MACACCESS_DEST_IDX_SET(pgid) |
> >              ANA_MACACCESS_ENTRYTYPE_SET(type) |
> >              ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_LEARN),
> >              lan966x, ANA_MACACCESS);
> > @@ -86,6 +88,27 @@ int lan966x_mac_learn(struct lan966x *lan966x, int port,
> >       return lan966x_mac_wait_for_completion(lan966x);
> >  }
> >
> > +int lan966x_mac_ip_learn(struct lan966x *lan966x,
> > +                      bool cpu_copy,
> > +                      const unsigned char mac[ETH_ALEN],
> > +                      unsigned int vid,
> > +                      enum macaccess_entry_type type)
> 
> I think it's worth mentioning in a comment above this function that the
> mask of front ports should be encoded into the address by now, via a
> call to lan966x_mdb_encode_mac().

Yes, I will do that.

> 
> > +{
> > +     WARN_ON(type != ENTRYTYPE_MACV4 && type != ENTRYTYPE_MACV6);
> > +
> > +     return __lan966x_mac_learn(lan966x, 0, cpu_copy, mac, vid, type);
> > +}
> > +
> > +int lan966x_mac_learn(struct lan966x *lan966x, int port,
> > +                   const unsigned char mac[ETH_ALEN],
> > +                   unsigned int vid,
> > +                   enum macaccess_entry_type type)
> > +{
> > +     WARN_ON(type != ENTRYTYPE_NORMAL && type != ENTRYTYPE_LOCKED);
> > +
> > +     return __lan966x_mac_learn(lan966x, port, false, mac, vid, type);
> > +}
> > +
> >  int lan966x_mac_forget(struct lan966x *lan966x,
> >                      const unsigned char mac[ETH_ALEN],
> >                      unsigned int vid,
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index c399b1256edc..f70e54526f53 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -157,6 +157,11 @@ int lan966x_port_pcs_set(struct lan966x_port *port,
> >                        struct lan966x_port_config *config);
> >  void lan966x_port_init(struct lan966x_port *port);
> >
> > +int lan966x_mac_ip_learn(struct lan966x *lan966x,
> > +                      bool cpu_copy,
> > +                      const unsigned char mac[ETH_ALEN],
> > +                      unsigned int vid,
> > +                      enum macaccess_entry_type type);
> >  int lan966x_mac_learn(struct lan966x *lan966x, int port,
> >                     const unsigned char mac[ETH_ALEN],
> >                     unsigned int vid,
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
> > index a13c469e139a..797560172aca 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
> > @@ -169,6 +169,12 @@ enum lan966x_target {
> >  #define ANA_MACACCESS_CHANGE2SW_GET(x)\
> >       FIELD_GET(ANA_MACACCESS_CHANGE2SW, x)
> >
> > +#define ANA_MACACCESS_MAC_CPU_COPY               BIT(16)
> > +#define ANA_MACACCESS_MAC_CPU_COPY_SET(x)\
> > +     FIELD_PREP(ANA_MACACCESS_MAC_CPU_COPY, x)
> > +#define ANA_MACACCESS_MAC_CPU_COPY_GET(x)\
> > +     FIELD_GET(ANA_MACACCESS_MAC_CPU_COPY, x)
> 
> Could you please add a space between (x) and \.

Actually I prefer for now not to do that.
The reason is that in the entire file there is no space between (x) and
\.
What I can do is when I will add another feature that requires more
registes then I can change the entire file to have that space if it is
worth it.

> 
> > +
> >  #define ANA_MACACCESS_VALID                      BIT(12)
> >  #define ANA_MACACCESS_VALID_SET(x)\
> >       FIELD_PREP(ANA_MACACCESS_VALID, x)
> > --
> > 2.33.0
> >

-- 
/Horatiu