Message-ID: <c66c8bd1-940a-bf9d-ce33-5a39635e9f5b@gmail.com>
Date:   Sat, 25 Sep 2021 19:39:34 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Andrew Lunn <andrew@...n.ch>, 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



On 9/25/2021 9:45 AM, Andrew Lunn wrote:
[snip]

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

Indeed, thanks.

[snip]

>> +	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()?

Yes we do need to undo the preceding clk_prepare_enable(), thanks!

[snip]

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

I had not given much thought about it first but we ought to do something 
about it here, I will test it before Justin sends a v2.

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

Yes, thanks!

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

We could do that, especially since one of the ports will typically 
connect to an external Gigabit PHY, will add for v2.

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

Wake-on-LAN using Magic Packets and network filters requires keeping the 
UniMAC's receiver turned on, and then the packets feed into the Magic 
Packet Detector (MPD) block or the network filter block. In that mode 
DRAM is in self refresh and there is local matching of frames into a 
tiny FIFO however in the case of magic packets the packets leading to a 
wake-up are dropped as there is nowhere to store them. In the case of a 
network filter match (e.g.: matching a multicast IP address plus 
protocol, plus source/destination ports) the packets are also discarded 
because the receive DMA was shut down.

When the wol_keep_rx_en flag is set, the above happens but we also allow 
the packets that did match a network filter to reach the small FIFO 
(Justin would know how many entries are there) that is used to push the 
packets to DRAM. The packet contents are held in there until the system 
wakes up which is usually just a few hundreds of micro seconds after we 
received a packet that triggered a wake-up. Once we overflow the receive 
DMA FIFO capacity subsequent packets get dropped which is fine since we 
are usually talking about very low bit rates, and we only try to push to 
DRAM the packets of interest, that is those for which we have a network 
filter.

This is convenient in scenarios where you want to wake-up from multicast 
DNS (e.g.: wake on Googlecast, Bonjour etc.) because then the packet 
that resulted in the system wake-up is not discarded but is then 
delivered to the network stack.

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

It is not, this has been copied from the GENET driver which also does 
not support 2.5Gbits avec the external interface level, we should drop 
it there, too. Thanks!

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

We can probably remove these comparisons nowadays since the PHY library 
no longer calls adjust_link whether or not something has changed, it 
does call when something changed.

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

Yes indeed.

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

OK.

[snip]

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

Yes this is a revision of the silicon that is not meant to see the light 
of day.
-- 
Florian

Powered by blists - more mailing lists