On Fri, Jan 24, 2025 at 05:53:45PM +0530, Basharath Hussain Khaja wrote:

quoted

From: Roger Quadros <redacted>

Updates Kernel configuration to enable PRUETH driver and its dependencies
along with makefile changes to add the new PRUETH driver.

Changes includes init and deinit of ICSSM PRU Ethernet driver including
net dev registration and firmware loading for DUAL-MAC mode running on
PRU-ICSS2 instance.

Changes also includes link handling, PRU booting, default firmware loading
and PRU stopping using existing remoteproc driver APIs.

Signed-off-by: Roger Quadros <redacted>
Signed-off-by: Andrew F. Davis <redacted>
Signed-off-by: Parvathi Pudi <parvathi@couthit.com>
Signed-off-by: Basharath Hussain Khaja <redacted>

...

quoted

diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c

b/drivers/net/ethernet/ti/icssm/icssm_prueth.c

...

quoted

+static int icssm_emac_set_boot_pru(struct prueth_emac *emac,
+				   struct net_device *ndev)
+{
+	const struct prueth_firmware *pru_firmwares;
+	struct prueth *prueth = emac->prueth;
+	const char *fw_name;
+	int ret;
+
+	pru_firmwares = &prueth->fw_data->fw_pru[emac->port_id - 1];
+	fw_name = pru_firmwares->fw_name[prueth->eth_type];
+	if (!fw_name) {
+		netdev_err(ndev, "eth_type %d not supported\n",
+			   prueth->eth_type);
+		return -ENODEV;
+	}
+
+	ret = rproc_set_firmware(emac->pru, fw_name);
+	if (ret) {
+		netdev_err(ndev, "failed to set PRU0 firmware %s: %d\n",
+			   fw_name, ret);
+		return ret;
+	}
+
+	ret = rproc_boot(emac->pru);
+	if (ret) {
+		netdev_err(ndev, "failed to boot PRU0: %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+/**
+ * icssm_emac_ndo_open - EMAC device open
+ * @ndev: network adapter device
+ *
+ * Called when system wants to start the interface.
+ *
+ * Return: 0 for a successful open, or appropriate error code
+ */
+static int icssm_emac_ndo_open(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	int ret;
+
+	ret = icssm_emac_set_boot_pru(emac, ndev);
+	if (ret)
+		netdev_err(ndev, "failed to boot PRU: %d\n", ret);

Hi Roger, Basharath, all,

icssm_emac_set_boot_pru() already logs errors, including the one above.
So this log seems unnecessary to me.

Also, should an error be returned here?  If so, it looks like
icssm_emac_set_boot_pru() should release resources allocated by
rproc_set_firmware() if rproc_boot() fails.

Agreed. We will remove the extra print and make sure allocated resources
are released appropriately in the next version.

quoted

+
+	/* start PHY */
+	phy_start(emac->phydev);
+
+	return 0;
+}

...

quoted

+static int icssm_prueth_netdev_init(struct prueth *prueth,
+				    struct device_node *eth_node)
+{
+	struct prueth_emac *emac;
+	struct net_device *ndev;
+	enum prueth_port port;
+	enum prueth_mac mac;
+	int ret;
+
+	port = icssm_prueth_node_port(eth_node);
+	if (port == PRUETH_PORT_INVALID)
+		return -EINVAL;
+
+	mac = icssm_prueth_node_mac(eth_node);
+	if (mac == PRUETH_MAC_INVALID)
+		return -EINVAL;
+
+	ndev = devm_alloc_etherdev(prueth->dev, sizeof(*emac));
+	if (!ndev)
+		return -ENOMEM;
+
+	SET_NETDEV_DEV(ndev, prueth->dev);
+	emac = netdev_priv(ndev);
+	prueth->emac[mac] = emac;
+	emac->prueth = prueth;
+	emac->ndev = ndev;
+	emac->port_id = port;
+
+	/* by default eth_type is EMAC */
+	switch (port) {
+	case PRUETH_PORT_MII0:
+		emac->pru = prueth->pru0;
+		break;
+	case PRUETH_PORT_MII1:
+		emac->pru = prueth->pru1;
+		break;
+	default:
+		return -EINVAL;
+	}
+	/* get mac address from DT and set private and netdev addr */
+	ret = of_get_ethdev_address(eth_node, ndev);
+	if (!is_valid_ether_addr(ndev->dev_addr)) {
+		eth_hw_addr_random(ndev);
+		dev_warn(prueth->dev, "port %d: using random MAC addr: %pM\n",
+			 port, ndev->dev_addr);
+	}
+	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
+
+	/* connect PHY */
+	emac->phydev = of_phy_get_and_connect(ndev, eth_node,
+					      icssm_emac_adjust_link);
+	if (!emac->phydev) {
+		dev_dbg(prueth->dev, "PHY connection failed\n");
+		ret = -EPROBE_DEFER;

Perhaps I misunderstand things, but if this occurs then
presumably icssm_prueth_netdev_init() will be called again.
And for each time this occirs another ndev will be allocated
by devm_alloc_etherdev(), each of which will only be freed
once the device is eventually torn-down.

I wonder if it would be better to free ndev here.
Which I think would imply using a non-mdev allocation for symmetry.

Similarly for resources allocated in the caller icssm_prueth_probe().

Agreed, we will address this error case and free the resources appropriately.

quoted

+		goto free;
+	}
+
+	/* remove unsupported modes */
+	phy_remove_link_mode(emac->phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
+
+	phy_remove_link_mode(emac->phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
+	phy_remove_link_mode(emac->phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+
+	phy_remove_link_mode(emac->phydev, ETHTOOL_LINK_MODE_Pause_BIT);
+	phy_remove_link_mode(emac->phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
+
+	ndev->netdev_ops = &emac_netdev_ops;
+
+	return 0;
+free:

nit: This doesn't free anything.

Sure. We will address in the next version.

quoted

+	prueth->emac[mac] = NULL;
+
+	return ret;
+}

...

Thanks & Best Regards,
Basharath