Date:   Sat, 25 Sep 2021 18:45:18 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Justin Chen <justinpopo6@...il.com>
Cc:     netdev@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Doug Berger <opendmb@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian König <christian.koenig@....com>,
        Rafał Miłecki <rafal@...ecki.pl>,
        Randy Dunlap <rdunlap@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Michael Chan <michael.chan@...adcom.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:DMA BUFFER SHARING FRAMEWORK" 
        <linux-media@...r.kernel.org>,
        "open list:DMA BUFFER SHARING FRAMEWORK" 
        <dri-devel@...ts.freedesktop.org>,
        "moderated list:DMA BUFFER SHARING FRAMEWORK" 
        <linaro-mm-sig@...ts.linaro.org>
Subject: Re: [PATCH net-next 3/5] net: bcmasp: Add support for ASP2.0
 Ethernet controller

> +static int bcmasp_probe(struct platform_device *pdev)
> +{
> +	struct bcmasp_priv *priv;
> +	struct device_node *ports_node, *intf_node;
> +	struct device *dev = &pdev->dev;
> +	int ret, i, wol_irq, count = 0;
> +	struct bcmasp_intf *intf;
> +	struct resource *r;
> +	u32 u32_reserved_filters_bitmask;
> +	DECLARE_BITMAP(reserved_filters_bitmask, ASP_RX_NET_FILTER_MAX);
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq <= 0) {
> +		dev_err(dev, "invalid interrupt\n");
> +		return -EINVAL;
> +	}
> +
> +	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;
> +	}

devm_clk_get_optional() makes this simpler/

> +
> +	/* Base from parent node */
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, r);
> +	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);
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable all clocks to ensure successful probing */
> +	bcmasp_core_clock_set(priv, ASP_CTRL_CLOCK_CTRL_ASP_ALL_DISABLE, 0);
> +
> +	/* Switch to the main clock */
> +	bcmasp_core_clock_select(priv, false);
> +
> +	_intr2_mask_set(priv, 0xffffffff);
> +	intr2_core_wl(priv, 0xffffffff, ASP_INTR2_CLEAR);
> +
> +	ret = devm_request_irq(&pdev->dev, priv->irq, bcmasp_isr, 0,
> +			       pdev->name, priv);
> +	if (ret) {
> +		dev_err(dev, "failed to request ASP interrupt: %d\n", ret);
> +		return ret;
> +	}

Do you need to undo clk_prepare_enable()? 

> +
> +	/* Register mdio child nodes */
> +	of_platform_populate(dev->of_node, bcmasp_mdio_of_match, NULL,
> +			     dev);
> +
> +	ret = of_property_read_u32(dev->of_node,
> +				   "brcm,reserved-net-filters-mask",
> +				   &u32_reserved_filters_bitmask);
> +	if (ret)
> +		u32_reserved_filters_bitmask = 0;
> +
> +	priv->net_filters_count_max = ASP_RX_NET_FILTER_MAX;
> +	bitmap_zero(reserved_filters_bitmask, priv->net_filters_count_max);
> +	bitmap_from_arr32(reserved_filters_bitmask,
> +			  &u32_reserved_filters_bitmask,
> +			  priv->net_filters_count_max);
> +
> +	/* Discover bitmask of reserved filters */
> +	for_each_set_bit(i, reserved_filters_bitmask, ASP_RX_NET_FILTER_MAX) {
> +		priv->net_filters[i].reserved = true;
> +		priv->net_filters_count_max--;
> +	}
> +
> +	/*
> +	 * ASP specific initialization, Needs to be done irregardless of
> +	 * of how many interfaces come up.
> +	 */
> +	bcmasp_core_init(priv);
> +	bcmasp_core_init_filters(priv);
> +
> +	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
> +	if (!ports_node) {
> +		dev_warn(dev, "No ports found\n");
> +		return 0;
> +	}
> +
> +	priv->intf_count = of_get_available_child_count(ports_node);
> +
> +	priv->intfs = devm_kcalloc(dev, priv->intf_count,
> +				   sizeof(struct bcmasp_intf *),
> +				   GFP_KERNEL);
> +	if (!priv->intfs)
> +		return -ENOMEM;
> +
> +	/* Probe each interface (Initalization should continue even if
> +	 * interfaces are unable to come up)
> +	 */
> +	i = 0;
> +	for_each_available_child_of_node(ports_node, intf_node) {
> +		wol_irq = platform_get_irq_optional(pdev, i + 1);
> +		priv->intfs[i++] = bcmasp_interface_create(priv, intf_node,
> +							   wol_irq);
> +	}
> +
> +	/* Drop the clock reference count now and let ndo_open()/ndo_close()
> +	 * manage it for us from now on.
> +	 */
> +	bcmasp_core_clock_set(priv, 0, ASP_CTRL_CLOCK_CTRL_ASP_ALL_DISABLE);
> +
> +	clk_disable_unprepare(priv->clk);
> +
> +	/* Now do the registration of the network ports which will take care of
> +	 * managing the clock properly.
> +	 */
> +	for (i = 0; i < priv->intf_count; i++) {
> +		intf = priv->intfs[i];
> +		if (!intf)
> +			continue;
> +
> +		ret = register_netdev(intf->ndev);
> +		if (ret) {
> +			netdev_err(intf->ndev,
> +				   "failed to register net_device: %d\n", ret);
> +			bcmasp_interface_destroy(intf, false);
> +			continue;
> +		}
> +		count++;
> +	}
> +
> +	dev_info(dev, "Initialized %d port(s)\n", count);
> +
> +	return 0;
> +}
> +
> +static int bcmasp_remove(struct platform_device *pdev)
> +{
> +	struct bcmasp_priv *priv = dev_get_drvdata(&pdev->dev);
> +	struct bcmasp_intf *intf;
> +	int i;
> +
> +	for (i = 0; i < priv->intf_count; i++) {
> +		intf = priv->intfs[i];
> +		if (!intf)
> +			continue;
> +
> +		bcmasp_interface_destroy(intf, true);
> +	}
> +
> +	return 0;
> +}

Do you need to depopulate the mdio children?

> +static void bcmasp_get_drvinfo(struct net_device *dev,
> +			       struct ethtool_drvinfo *info)
> +{
> +	strlcpy(info->driver, "bcmasp", sizeof(info->driver));
> +	strlcpy(info->version, "v2.0", sizeof(info->version));

Please drop version. The core will fill it in with the kernel version,
which is more useful.

> +static int bcmasp_nway_reset(struct net_device *dev)
> +{
> +	if (!dev->phydev)
> +		return -ENODEV;
> +
> +	return genphy_restart_aneg(dev->phydev);
> +}

phy_ethtool_nway_reset().


> +static void bcmasp_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> +{
> +	struct bcmasp_intf *intf = netdev_priv(dev);
> +
> +	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
> +	wol->wolopts = intf->wolopts;
> +	memset(wol->sopass, 0, sizeof(wol->sopass));
> +
> +	if (wol->wolopts & WAKE_MAGICSECURE)
> +		memcpy(wol->sopass, intf->sopass, sizeof(intf->sopass));
> +}

Maybe consider calling into the PHY to see what it can do? If the PHY
can do the WoL you want, it will do it with less power.

> +static int bcmasp_set_priv_flags(struct net_device *dev, u32 flags)
> +{
> +	struct bcmasp_intf *intf = netdev_priv(dev);
> +
> +	intf->wol_keep_rx_en = flags & BCMASP_WOL_KEEP_RX_EN ? 1 : 0;
> +
> +	return 0;

Please could you explain this some more. How can you disable RX and
still have WoL working?

> +static void bcmasp_adj_link(struct net_device *dev)
> +{
> +	struct bcmasp_intf *intf = netdev_priv(dev);
> +	struct phy_device *phydev = dev->phydev;
> +	int changed = 0;
> +	u32 cmd_bits = 0, reg;
> +
> +	if (intf->old_link != phydev->link) {
> +		changed = 1;
> +		intf->old_link = phydev->link;
> +	}
> +
> +	if (intf->old_duplex != phydev->duplex) {
> +		changed = 1;
> +		intf->old_duplex = phydev->duplex;
> +	}
> +
> +	switch (phydev->speed) {
> +	case SPEED_2500:
> +		cmd_bits = UMC_CMD_SPEED_2500;

All i've seen is references to RGMII. Is 2500 possible?

> +		break;
> +	case SPEED_1000:
> +		cmd_bits = UMC_CMD_SPEED_1000;
> +		break;
> +	case SPEED_100:
> +		cmd_bits = UMC_CMD_SPEED_100;
> +		break;
> +	case SPEED_10:
> +		cmd_bits = UMC_CMD_SPEED_10;
> +		break;
> +	default:
> +		break;
> +	}
> +	cmd_bits <<= UMC_CMD_SPEED_SHIFT;
> +
> +	if (phydev->duplex == DUPLEX_HALF)
> +		cmd_bits |= UMC_CMD_HD_EN;
> +
> +	if (intf->old_pause != phydev->pause) {
> +		changed = 1;
> +		intf->old_pause = phydev->pause;
> +	}
> +
> +	if (!phydev->pause)
> +		cmd_bits |= UMC_CMD_RX_PAUSE_IGNORE | UMC_CMD_TX_PAUSE_IGNORE;
> +
> +	if (!changed)
> +		return;

Shouldn't there be a comparison intd->old_speed != phydev->speed?  You
are risking the PHY can change speed without doing a link down/up?

> +
> +	if (phydev->link) {
> +		reg = umac_rl(intf, UMC_CMD);
> +		reg &= ~((UMC_CMD_SPEED_MASK << UMC_CMD_SPEED_SHIFT) |
> +			UMC_CMD_HD_EN | UMC_CMD_RX_PAUSE_IGNORE |
> +			UMC_CMD_TX_PAUSE_IGNORE);
> +		reg |= cmd_bits;
> +		umac_wl(intf, reg, UMC_CMD);
> +
> +		/* Enable RGMII pad */
> +		reg = rgmii_rl(intf, RGMII_OOB_CNTRL);
> +		reg |= RGMII_MODE_EN;
> +		rgmii_wl(intf, reg, RGMII_OOB_CNTRL);
> +
> +		intf->eee.eee_active = phy_init_eee(phydev, 0) >= 0;
> +		bcmasp_eee_enable_set(intf, intf->eee.eee_active);
> +	} else {
> +		/* Disable RGMII pad */
> +		reg = rgmii_rl(intf, RGMII_OOB_CNTRL);
> +		reg &= ~RGMII_MODE_EN;
> +		rgmii_wl(intf, reg, RGMII_OOB_CNTRL);
> +	}
> +
> +	if (changed)
> +		phy_print_status(phydev);

There has already been a return if !changed.

> +static void bcmasp_configure_port(struct bcmasp_intf *intf)
> +{
> +	u32 reg, id_mode_dis = 0;
> +
> +	reg = rgmii_rl(intf, RGMII_PORT_CNTRL);
> +	reg &= ~RGMII_PORT_MODE_MASK;
> +
> +	switch (intf->phy_interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* RGMII_NO_ID: TXC transitions at the same time as TXD
> +		 *		(requires PCB or receiver-side delay)
> +		 * RGMII:	Add 2ns delay on TXC (90 degree shift)
> +		 *
> +		 * ID is implicitly disabled for 100Mbps (RG)MII operation.
> +		 */
> +		id_mode_dis = RGMII_ID_MODE_DIS;
> +		fallthrough;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		reg |= RGMII_PORT_MODE_EXT_GPHY;
> +		break;
> +	case PHY_INTERFACE_MODE_MII:
> +		reg |= RGMII_PORT_MODE_EXT_EPHY;
> +		break;
> +	default:
> +		break;
> +	}

Can we skip this and let the PHY do the delays? Ah, "This is an ugly
quirk..." Maybe add a comment here pointing towards
bcmasp_netif_init(), which is explains this.

> +static int bcmasp_netif_init(struct net_device *dev, bool phy_connect,
> +			     bool init_rx)
> +{
> +	struct bcmasp_intf *intf = netdev_priv(dev);
> +	phy_interface_t phy_iface = intf->phy_interface;
> +	u32 phy_flags = PHY_BRCM_AUTO_PWRDWN_ENABLE |
> +			PHY_BRCM_DIS_TXCRXC_NOENRGY |
> +			PHY_BRCM_IDDQ_SUSPEND;
> +	struct phy_device *phydev = NULL;
> +	int ret;
> +
> +	/* Always enable interface clocks */
> +	bcmasp_core_clock_set_intf(intf, true);
> +
> +	/* Enable internal PHY before any MAC activity */
> +	if (intf->internal_phy)
> +		bcmasp_ephy_enable_set(intf, true);
> +
> +	bcmasp_configure_port(intf);
> +
> +	/* This is an ugly quirk but we have not been correctly interpreting
> +	 * the phy_interface values and we have done that across different
> +	 * drivers, so at least we are consistent in our mistakes.
> +	 *
> +	 * When the Generic PHY driver is in use either the PHY has been
> +	 * strapped or programmed correctly by the boot loader so we should
> +	 * stick to our incorrect interpretation since we have validated it.
> +	 *
> +	 * Now when a dedicated PHY driver is in use, we need to reverse the
> +	 * meaning of the phy_interface_mode values to something that the PHY
> +	 * driver will interpret and act on such that we have two mistakes
> +	 * canceling themselves so to speak. We only do this for the two
> +	 * modes that GENET driver officially supports on Broadcom STB chips:
> +	 * PHY_INTERFACE_MODE_RGMII and PHY_INTERFACE_MODE_RGMII_TXID. Other
> +	 * modes are not *officially* supported with the boot loader and the
> +	 * scripted environment generating Device Tree blobs for those
> +	 * platforms.
> +	 *
> +	 * Note that internal PHY and fixed-link configurations are not
> +	 * affected because they use different phy_interface_t values or the
> +	 * Generic PHY driver.
> +	 */


> +static inline void bcmasp_map_res(struct bcmasp_priv *priv,
> +				  struct bcmasp_intf *intf)
> +{
> +	/* Per port */
> +	intf->res.umac = priv->base + UMC_OFFSET(intf);
> +	intf->res.umac2fb = priv->base + UMAC2FB_OFFSET(intf);
> +	intf->res.rgmii = priv->base + RGMII_OFFSET(intf);
> +
> +	/* Per ch */
> +	intf->tx_spb_dma = priv->base + TX_SPB_DMA_OFFSET(intf);
> +	intf->res.tx_spb_ctrl = priv->base + TX_SPB_CTRL_OFFSET(intf);
> +	/*
> +	 * Stop gap solution. This should be removed when 72165a0 is
> +	 * deprecated
> +	 */

Is that an internal commit?

   Andrew

Powered by blists - more mailing lists