[Linaro-mm-sig] Re: [PATCH net-next 3/6] net: bcmasp: Add support for ASP2.0 Ethernet controller

On Tue, Apr 18, 2023 at 05:10:15PM -0700, Justin Chen wrote:

Add support for the Broadcom ASP 2.0 Ethernet controller which is first introduced with 72165. This controller features two distinct Ethernet ports that can be independently operated.

This patch supports:

• Wake-on-LAN using magic packets
• basic ethtool operations (link, counters, message level)
• MAC destination address filtering (promiscuous, ALL_MULTI, etc.)

Signed-off-by: Justin Chen justinpopo6@gmail.com Signed-off-by: Florian Fainelli f.fainelli@gmail.com

Hi Justin

Since you are submitting this, your Signed-off-by: needs to be last here.

---

drivers/net/ethernet/broadcom/Kconfig | 11 + drivers/net/ethernet/broadcom/Makefile | 1 + drivers/net/ethernet/broadcom/asp2/Makefile | 2 + drivers/net/ethernet/broadcom/asp2/bcmasp.c | 1527 ++++++++++++++++++++ drivers/net/ethernet/broadcom/asp2/bcmasp.h | 636 ++++++++ .../net/ethernet/broadcom/asp2/bcmasp_ethtool.c | 620 ++++++++ drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c | 1425 ++++++++++++++++++ .../net/ethernet/broadcom/asp2/bcmasp_intf_defs.h | 238 +++ 8 files changed, 4460 insertions(+) create mode 100644 drivers/net/ethernet/broadcom/asp2/Makefile create mode 100644 drivers/net/ethernet/broadcom/asp2/bcmasp.c create mode 100644 drivers/net/ethernet/broadcom/asp2/bcmasp.h create mode 100644 drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c create mode 100644 drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c create mode 100644 drivers/net/ethernet/broadcom/asp2/bcmasp_intf_defs.h

diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig index 948586bf1b5b..d4166141145d 100644 --- a/drivers/net/ethernet/broadcom/Kconfig +++ b/drivers/net/ethernet/broadcom/Kconfig @@ -255,4 +255,15 @@ config BNXT_HWMON Say Y if you want to expose the thermal sensor data on NetXtreme-C/E devices, via the hwmon sysfs interface. +config BCMASP

• tristate "Broadcom ASP 2.0 Ethernet support"
• default ARCH_BRCMSTB
• depends on OF
• select MII
• select PHYLIB
• select MDIO_BCM_UNIMAC
• help

•  This configuration enables the Broadcom ASP 2.0 Ethernet controller
  

•  driver which is present in Broadcom STB SoCs such as 72165.
  

endif # NET_VENDOR_BROADCOM diff --git a/drivers/net/ethernet/broadcom/Makefile b/drivers/net/ethernet/broadcom/Makefile index 0ddfb5b5d53c..bac5cb6ad0cd 100644 --- a/drivers/net/ethernet/broadcom/Makefile +++ b/drivers/net/ethernet/broadcom/Makefile @@ -17,3 +17,4 @@ obj-$(CONFIG_BGMAC_BCMA) += bgmac-bcma.o bgmac-bcma-mdio.o obj-$(CONFIG_BGMAC_PLATFORM) += bgmac-platform.o obj-$(CONFIG_SYSTEMPORT) += bcmsysport.o obj-$(CONFIG_BNXT) += bnxt/ +obj-$(CONFIG_BCMASP) += asp2/ diff --git a/drivers/net/ethernet/broadcom/asp2/Makefile b/drivers/net/ethernet/broadcom/asp2/Makefile new file mode 100644 index 000000000000..e07550315f83 --- /dev/null +++ b/drivers/net/ethernet/broadcom/asp2/Makefile @@ -0,0 +1,2 @@ +obj-$(CONFIG_BCMASP) += bcm-asp.o +bcm-asp-objs := bcmasp.o bcmasp_intf.o bcmasp_ethtool.o diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c new file mode 100644 index 000000000000..9cf5f4d6dd0d --- /dev/null +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c @@ -0,0 +1,1527 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Broadcom STB ASP 2.0 Driver
• • Copyright (c) 2020 Broadcom
• */

+#include <linux/etherdevice.h> +#include <linux/if_vlan.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/clk.h>

+#include "bcmasp.h" +#include "bcmasp_intf_defs.h"

+static inline void _intr2_mask_clear(struct bcmasp_priv *priv, u32 mask) +{

• intr2_core_wl(priv, mask, ASP_INTR2_MASK_CLEAR);
• priv->irq_mask &= ~mask;

+}

No inline functions in .c files. Let the compiler decide.

+static inline void bcmasp_intr2_handling(struct bcmasp_intf *intf, u32 status) +{

• if (unlikely(!intf))

• return;
  

Can it even happen? An interrupt from an interface which does not exist?

+static void bcmasp_set_mda_filter(struct bcmasp_intf *intf,

• 		  const unsigned char *addr,
  

• 		  unsigned char *mask,
  

• 		  unsigned int i)
  

+{

• struct bcmasp_priv *priv = intf->parent;
• u32 addr_h, addr_l, mask_h, mask_l;
• /* Set local copy */
• memcpy(priv->mda_filters[i].mask, mask, ETH_ALEN);
• memcpy(priv->mda_filters[i].addr, addr, ETH_ALEN);

eth_addr_copy() ?

+static inline void u64_to_mac(unsigned char *addr, u64 val) +{

• addr[0] = (u8)(val >> 40);
• addr[1] = (u8)(val >> 32);
• addr[2] = (u8)(val >> 24);
• addr[3] = (u8)(val >> 16);
• addr[4] = (u8)(val >> 8);
• addr[5] = (u8)val;

+}

u64_to_ether_addr() ?

+#define mac_to_u64(a) ((((u64)a[0]) << 40) | \

• 		(((u64)a[1]) << 32) | \
  

• 		(((u64)a[2]) << 24) | \
  

• 		(((u64)a[3]) << 16) | \
  

• 		(((u64)a[4]) << 8) | \
  

• 		((u64)a[5]))
  

ether_addr_to_u64()

You might want to read that include file and see if there is anything else you can replace.

+static int bcmasp_probe(struct platform_device *pdev) +{

• struct bcmasp_priv *priv;
• struct device_node *ports_node, *intf_node;
• struct device *dev = &pdev->dev;
• const struct bcmasp_plat_data *pdata;
• int ret, i, count = 0, port;
• struct bcmasp_intf *intf;

Reverse christmass tree.

• priv->clk = devm_clk_get(dev, "sw_asp");
• if (IS_ERR(priv->clk)) {

• if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
  

• 	return -EPROBE_DEFER;
  

• dev_warn(dev, "failed to request clock\n");
  

• priv->clk = NULL;
  

• }

Maybe devm_clk_get_optional() ??

• /* Base from parent node */
• priv->base = devm_platform_ioremap_resource(pdev, 0);
• if (IS_ERR(priv->base)) {

• dev_err(dev, "failed to iomap\n");
  

• return PTR_ERR(priv->base);
  

• }
• ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
• if (ret)

• ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
  

• if (ret) {

• dev_err(&pdev->dev, "unable to set DMA mask: %d\n", ret);
  

• return ret;
  

• }
• dev_set_drvdata(&pdev->dev, priv);
• priv->pdev = pdev;
• spin_lock_init(&priv->mda_lock);
• spin_lock_init(&priv->clk_lock);
• mutex_init(&priv->net_lock);
• mutex_init(&priv->wol_lock);
• pdata = device_get_match_data(&pdev->dev);
• if (pdata) {

• priv->init_wol = pdata->init_wol;
  

• priv->enable_wol = pdata->enable_wol;
  

• priv->destroy_wol = pdata->destroy_wol;
  

• priv->hw_info = pdata->hw_info;
  

• } else {

• dev_err(&pdev->dev, "unable to find platform data\n");
  

• return ret;
  

• }
• ret = clk_prepare_enable(priv->clk);
• if (ret)

• return ret;
  

I think there is also a devm_clk_get_enable_optional().

+static void bcmasp_get_drvinfo(struct net_device *dev,

• 	       struct ethtool_drvinfo *info)
  

+{

• strscpy(info->driver, "bcmasp", sizeof(info->driver));
• strscpy(info->version, "v2.0", sizeof(info->version));

Don't fill in the version, it is useless. The core will insert the git hash which has more value.

• strscpy(info->bus_info, dev_name(dev->dev.parent),

• sizeof(info->bus_info));
  

+}

+static int bcmasp_get_link_ksettings(struct net_device *dev,

• 		     struct ethtool_link_ksettings *cmd)
  

+{

• if (!netif_running(dev))

• return -EINVAL;
  

• if (!dev->phydev)

• return -ENODEV;
  

I skipped the PHY handling section. Is it possible to not have a PHY? Normally you have a fixed-link if their is not a real PHY.

There is also phy_ethtool_set_link_ksettings()

+static int bcmasp_nway_reset(struct net_device *dev) +{

• if (!dev->phydev)

• return -ENODEV;
  

• return genphy_restart_aneg(dev->phydev);

phy_ethtool_nway_reset().

I get the feeling this code was written against an old kernel version, and has not been updated to use all the new helpers.

Andrew