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

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@lunn.ch>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>; Niklas Söderlund
[off-list ref]; Paul Barker [off-list ref]; Andrew Lunn [off-list ref];
David S. Miller [off-list ref]; Eric Dumazet [off-list ref]; Jakub Kicinski
[off-list ref]; Paolo Abeni [off-list ref]; netdev@vger.kernel.org; linux-renesas-
soc@vger.kernel.org; linux-kernel@vger.kernel.org; Nikita Yushchenko [off-list ref]
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.

quoted

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Friday, July 4, 2025 10:44 AM
To: Michael Dege <redacted>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>; Niklas
Söderlund [off-list ref]; Paul Barker
[off-list ref]; Andrew Lunn [off-list ref]; David S.
Miller [off-list ref]; Eric Dumazet [off-list ref];
Jakub Kicinski [off-list ref]; Paolo Abeni [off-list ref];
netdev@vger.kernel.org; linux-renesas- soc@vger.kernel.org;
linux-kernel@vger.kernel.org; Nikita Yushchenko
[off-list ref]
Subject: Re: [PATCH 2/3] net: renesas: rswitch: add offloading for L2
switching

quoted

 #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.

quoted

quoted

@@ -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.

quoted

+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

quoted

quoted

+ 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.

quoted

quoted

@@ -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

quoted

    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.