Message-ID: <5fd25c58-b421-4ec0-8b4f-24f86f054a44@lunn.ch>
Date: Tue, 16 Apr 2024 00:55:12 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next] net: ethernet: rtsn: Add support for Renesas
 Ethernet-TSN

> +static int rtsn_get_phy_params(struct rtsn_private *priv)
> +{
> +	struct device_node *np = priv->ndev->dev.parent->of_node;
> +
> +	of_get_phy_mode(np, &priv->iface);
> +	switch (priv->iface) {
> +	case PHY_INTERFACE_MODE_MII:
> +		priv->speed = 100;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII:

There are 4 different RGMII modes, and you probably should be using
PHY_INTERFACE_MODE_RGMII_ID with the PHY. So you should list them all
here.

> +		priv->speed = 1000;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static void rtsn_set_phy_interface(struct rtsn_private *priv)
> +{
> +	u32 val;
> +
> +	switch (priv->iface) {
> +	case PHY_INTERFACE_MODE_MII:
> +		val = MPIC_PIS_MII;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII:

And here.

> +		val = MPIC_PIS_GMII;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	rtsn_modify(priv, MPIC, MPIC_PIS_MASK, val);
> +}
> +
> +static void rtsn_set_delay_mode(struct rtsn_private *priv)
> +{
> +	struct device_node *np = priv->ndev->dev.parent->of_node;
> +	u32 delay;
> +	u32 val;
> +
> +	val = 0;
> +
> +	/* Valid values are 0 and 1800, according to DT bindings */

The bindings should not matter. It is what the hardware supports. The
bindings should match the hardware, since it is hard to modify the
hardware to make it match the binding.

> +	if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay))
> +		if (delay)
> +			val |= GPOUT_RDM;
> +
> +	/* Valid values are 0 and 2000, according to DT bindings */
> +	if (!of_property_read_u32(np, "tx-internal-delay-ps", &delay))
> +		if (delay)
> +			val |= GPOUT_TDM;
> +
> +	rtsn_write(priv, GPOUT, val);

So you seem to be using it as bool? That is wrong. It is a number of
pico seconds!

> +static int rtsn_mii_access_indirect(struct mii_bus *bus, bool read, int phyad,
> +				    int devnum, int regnum, u16 data)
> +{
> +	int ret;
> +
> +	ret = rtsn_mii_access(bus, false, phyad, MII_MMD_CTRL, devnum);
> +	if (ret)
> +		return ret;
> +
> +	ret = rtsn_mii_access(bus, false, phyad, MII_MMD_DATA, regnum);
> +	if (ret)
> +		return ret;
> +
> +	ret = rtsn_mii_access(bus, false, phyad, MII_MMD_CTRL,
> +			      devnum | MII_MMD_CTRL_NOINCR);

This looks to be C45 over C22. phylib core knows how to do this, since
it should be the same for all PHYs which implement C45 over C22. So
there is no need for you to implement it again.

> +static int rtsn_mii_register(struct rtsn_private *priv)
> +{
> +	struct platform_device *pdev = priv->pdev;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *mdio_node;
> +	struct mii_bus *mii;
> +	int ret;
> +
> +	mii = mdiobus_alloc();
> +	if (!mii)
> +		return -ENOMEM;
> +
> +	mdio_node = of_get_child_by_name(dev->of_node, "mdio");
> +	if (!mdio_node) {
> +		ret = -ENODEV;
> +		goto out_free_bus;
> +	};
> +
> +	mii->name = "rtsn_mii";
> +	sprintf(mii->id, "%s-%x", pdev->name, pdev->id);
> +	mii->priv = priv;
> +	mii->read = rtsn_mii_read;
> +	mii->write = rtsn_mii_write;
> +	mii->read_c45 = rtsn_mii_read_c45;
> +	mii->write_c45 = rtsn_mii_write_c45;

Just leave these two empty, and the core will do C45 over C22 for you.

> +static void rtsn_phy_deinit(struct rtsn_private *priv)
> +{
> +	phy_stop(priv->ndev->phydev);

I would normally expect rtsn_phy_init() and rtsn_phy_deinit() to be
mirrors. You don't call phy_start() in rtsn_phy_init(), so why do you
call phy_stop() here? It probably should be somewhere else.

> +	phy_disconnect(priv->ndev->phydev);
> +	priv->ndev->phydev = NULL;
> +}


> +static int rtsn_open(struct net_device *ndev)
> +{
> +	struct rtsn_private *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	napi_enable(&priv->napi);
> +
> +	ret = rtsn_init(priv);
> +	if (ret) {
> +		napi_disable(&priv->napi);
> +		return ret;
> +	}
> +
> +	phy_start(ndev->phydev);
> +
> +	netif_start_queue(ndev);
> +
> +	return 0;
> +}
> +
> +static int rtsn_stop(struct net_device *ndev)
> +{
> +	struct rtsn_private *priv = netdev_priv(ndev);

This is probably where your phy_stop() belongs.

> +
> +	napi_disable(&priv->napi);
> +	rtsn_change_mode(priv, OCR_OPC_DISABLE);
> +	rtsn_deinit(priv);
> +
> +	return 0;
> +}

> +
> +static int rtsn_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
> +{
> +	if (!netif_running(ndev))
> +		return -EINVAL;
> +
> +	switch (cmd) {
> +	case SIOCGHWTSTAMP:
> +		return rtsn_hwstamp_get(ndev, req);
> +	case SIOCSHWTSTAMP:
> +		return rtsn_hwstamp_set(ndev, req);
> +	default:
> +		break;
> +	}
> +
> +	return 0;

Call phy_do_ioctl() rather than return 0. That allows the PHY driver
to handle its IOCTLs.

> +static int rtsn_probe(struct platform_device *pdev)
> +{
> +	struct rtsn_private *priv;
> +	struct net_device *ndev;
> +	struct resource *res;
> +	int ret;
> +
> +	ndev = alloc_etherdev_mqs(sizeof(struct rtsn_private), TX_NUM_CHAINS,
> +				  RX_NUM_CHAINS);
> +	if (!ndev)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(ndev);
> +	priv->pdev = pdev;
> +	priv->ndev = ndev;
> +	priv->ptp_priv = rcar_gen4_ptp_alloc(pdev);
> +
> +	spin_lock_init(&priv->lock);
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		ret = -PTR_ERR(priv->clk);
> +		goto error_alloc;
> +	}
> +
> +	priv->reset = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->reset)) {
> +		ret = -PTR_ERR(priv->reset);
> +		goto error_alloc;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tsnes");
> +	if (!res) {
> +		dev_err(&pdev->dev, "Can't find tsnes resource\n");
> +		ret = -EINVAL;
> +		goto error_alloc;
> +	}
> +
> +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->base)) {
> +		ret = PTR_ERR(priv->base);
> +		goto error_alloc;
> +	}
> +
> +	SET_NETDEV_DEV(ndev, &pdev->dev);
> +	ether_setup(ndev);
> +
> +	ndev->features = NETIF_F_RXCSUM;
> +	ndev->hw_features = NETIF_F_RXCSUM;
> +	ndev->base_addr = res->start;
> +	ndev->netdev_ops = &rtsn_netdev_ops;
> +	ndev->ethtool_ops = &rtsn_ethtool_ops;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gptp");
> +	if (!res) {
> +		dev_err(&pdev->dev, "Can't find gptp resource\n");
> +		ret = -EINVAL;
> +		goto error_alloc;
> +	}
> +	priv->ptp_priv->addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->ptp_priv->addr)) {
> +		ret = -PTR_ERR(priv->ptp_priv->addr);
> +		goto error_alloc;
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +
> +	netif_napi_add(ndev, &priv->napi, rtsn_poll);
> +
> +	rtsn_parse_mac_address(pdev->dev.of_node, ndev);
> +
> +	ret = register_netdev(ndev);
> +	if (ret)
> +		goto error_pm;
> +
> +	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

You need to be careful what you put after register_netdev(). The
kernel can be sending packets before register_netdev() even
returns. This can happen with NFS root, when the kernel will
immediately try to mount the root file system. Is it safe to handle
packets with the DMA mask set wrong?

	Andrew

Powered by blists - more mailing lists