Re: [PATCH net-next 3/5] net: bcmasp: Add support for ASP2.0 Ethernet controller

From: Florian Fainelli
Date: Sat Sep 25 2021 - 22:39:43 EST



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