RE: [PATCH 2/3] net: renesas: rswitch: add offloading for L2 switching

[Michael Dege]

Hello Andrew,

Short update on a few points.

> -----Original Message-----
> From: Michael Dege
> Sent: Monday, July 7, 2025 10:14 AM
> To: 'Andrew Lunn' <andrew@xxxxxxx>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>; Niklas Söderlund
> <niklas.soderlund@xxxxxxxxxxxx>; Paul Barker <paul@xxxxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>;
> David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski
> <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-renesas-
> soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH 2/3] net: renesas: rswitch: add offloading for L2 switching
>
> Hello Andrew,
>
> Thank you very much for your comments. I am currently figuring out how to take them into account.
>
> > -----Original Message-----
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Sent: Friday, July 4, 2025 10:44 AM
> > To: Michael Dege <michael.dege@xxxxxxxxxxx>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>; Niklas
> > Söderlund <niklas.soderlund@xxxxxxxxxxxx>; Paul Barker
> > <paul@xxxxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>; David S.
> > Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>;
> > Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>;
> > netdev@xxxxxxxxxxxxxxx; linux-renesas- soc@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; Nikita Yushchenko
> > <nikita.yoush@xxxxxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH 2/3] net: renesas: rswitch: add offloading for L2
> > switching
> >
> > >  #include <linux/platform_device.h>
> > > +#include <linux/phy.h>
> > > +
> >
> > It seems odd that a patch adding L2 support needs to touch PHYs?
>
> I will figure out where it was needed. Maybe I can get rid of it, or if needed I will move it to the
> File that needs it.

This include is needed because struct rswitch_etha has a member of type phy_interface. And rswitch_etha
is a member of struct rswitch_device. And rswitch_device is used also in rswich_l2.c.

>
> >
> > > @@ -994,10 +1018,18 @@ struct rswitch_device {
> > >   DECLARE_BITMAP(ts_skb_used, TS_TAGS_PER_PORT);
> > >   bool disabled;
> > >
> > > + struct list_head list;
> > > +
> > >   int port;
> > >   struct rswitch_etha *etha;
> > >   struct device_node *np_port;
> > >   struct phy *serdes;
> > > +
> > > + struct net_device *brdev;       /* master bridge device */
> >
> > How many ports does this device have? If it is just two, this might
> > work. But for a multi-port device, you need to keep this in the port structure.
> >
> > > +bool is_rdev(const struct net_device *ndev); void
> > > +rswitch_modify(void __iomem *addr, enum rswitch_reg reg, u32 clear,
> > > +u32 set);
> >
> > Are these actually needed? It seems like they could be local functions.
>
> Currently is_rdev() is only used in rswitch_l2.c. I moved it to that file and made it static. In the
> future it will also be used in the L3 routing. The function rswitch_modify() is used in rswitch_main.c
> and rswitch_l2.c I believe in this case it does make sense to have a single implementation. Or should
> I use two local copies?

The function is_rdev() is an accessor to rswitch_netdev_ops which is local
to rswitch_main.c. I believe it is better to provide this function globally
to the driver instead of the whole rswitch_netdev_ops structure.

Best regards,

Michael

>
> >
> > > + if (offload_brdev && !priv->offload_brdev)
> > > +         dev_info(&priv->pdev->dev, "starting l2 offload for %s\n",
> > > +                  netdev_name(offload_brdev));
> > > + else if (!offload_brdev && priv->offload_brdev)
> > > +         dev_info(&priv->pdev->dev, "stopping l2 offload for %s\n",
> > > +                  netdev_name(priv->offload_brdev));
> >
> > Please don't spam the log like this dev_dbg() maybe.
>
> I'll change that.
>
> >
> > > @@ -128,6 +134,14 @@ static void rswitch_fwd_init(struct rswitch_private *priv)
> > >           iowrite32(0, priv->addr + FWPBFC(i));
> > >   }
> > >
> > > + /* Configure MAC table aging */
> > > + rswitch_modify(priv->addr, FWMACAGUSPC, FWMACAGUSPC_MACAGUSP,
> > > +                FIELD_PREP(FWMACAGUSPC_MACAGUSP, 0x140));
> > > +
> > > + reg_val = FIELD_PREP(FWMACAGC_MACAGT, RSW_AGEING_TIME);
> > > + reg_val |= FWMACAGC_MACAGE | FWMACAGC_MACAGSL;
> > > + iowrite32(reg_val, priv->addr + FWMACAGC);
> > > +
> >
> > Please pull ageing out into a patch of its own.
>
> OK, will do that.
>
> Best regards,
>
> Michael
> >
> >     Andrew
> >
> > ---
> > pw-bot: cr
________________________________

Renesas Electronics Europe GmbH
Registered Office: Arcadiastrasse 10
DE-40472 Duesseldorf
Commercial Registry: Duesseldorf, HRB 3708
Managing Director: Carsten Jauch
VAT-No.: DE 14978647
Tax-ID-No: 105/5839/1793

Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.