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

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?

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

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