Re: [net-next,v2] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Andrew,

Thanks for your feedback.

On 2024-05-07 22:57:47 +0200, Andrew Lunn wrote:
> > +RENESAS ETHERNET TSN DRIVER
> > +M:	Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > +L:	netdev@vger.kernel.org
> > +L:	linux-renesas-soc@vger.kernel.org
> > +S:	Supported
> > +F:	Documentation/devicetree/bindings/net/renesas,ethertsn.yaml
> > +F:	drivers/net/ethernet/renesas/rtsn.*
> > +
> >  RENESAS IDT821034 ASoC CODEC
> >  M:	Herve Codina <herve.codina@bootlin.com>
> >  L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> > diff --git a/drivers/net/ethernet/renesas/Kconfig b/drivers/net/ethernet/renesas/Kconfig
> > index b03fae7a0f72..ea4aca5f406f 100644
> > --- a/drivers/net/ethernet/renesas/Kconfig
> > +++ b/drivers/net/ethernet/renesas/Kconfig
> > @@ -58,4 +58,15 @@ config RENESAS_GEN4_PTP
> >  	help
> >  	  Renesas R-Car Gen4 gPTP device driver.
> >  
> > +config RTSN
> > +	tristate "Renesas Ethernet-TSN support"
> > +	depends on ARCH_RENESAS || COMPILE_TEST
> > +	depends on PTP_1588_CLOCK
> > +	select CRC32
> > +	select MII
> 
> That is an interesting one. What are you using from MII?

Nothing it seems, this is cargo copied from other renesas drivers, will 
fix for next version.

> 
> > +static int rtsn_request_irq(unsigned int irq, irq_handler_t handler,
> > +			    unsigned long flags, struct rtsn_private *priv,
> > +			    const char *ch)
> > +{
> > +	char *name;
> > +	int ret;
> > +
> > +	name = devm_kasprintf(&priv->pdev->dev, GFP_KERNEL, "%s:%s",
> > +			      priv->ndev->name, ch);
> > +	if (!name)
> > +		return -ENOMEM;
> > +
> > +	ret = request_irq(irq, handler, flags, name, priv);
> > +	if (ret) {
> > +		netdev_err(priv->ndev, "Cannot request IRQ %s\n", name);
> > +		free_irq(irq, priv);
> 
> If requesting an IRQ fails, do you need to free it?

Will fix.

> 
> > +static void rtsn_free_irqs(struct rtsn_private *priv)
> > +{
> > +	free_irq(priv->tx_data_irq, priv);
> > +	free_irq(priv->rx_data_irq, priv);
> > +}
> > +
> > +static int rtsn_request_irqs(struct rtsn_private *priv)
> > +{
> > +	int ret;
> > +
> > +	priv->rx_data_irq = platform_get_irq_byname(priv->pdev, "rx");
> > +	if (priv->rx_data_irq < 0)
> > +		return priv->rx_data_irq;
> > +
> > +	priv->tx_data_irq = platform_get_irq_byname(priv->pdev, "tx");
> > +	if (priv->tx_data_irq < 0)
> > +		return priv->tx_data_irq;
> > +
> > +	ret = rtsn_request_irq(priv->tx_data_irq, rtsn_irq, 0, priv, "tx");
> > +	if (ret)
> > +		goto error;
> > +
> > +	ret = rtsn_request_irq(priv->rx_data_irq, rtsn_irq, 0, priv, "rx");
> > +	if (ret)
> > +		goto error;
> > +
> > +	return 0;
> > +error:
> > +	rtsn_free_irqs(priv);
> 
> This also looks to free IRQs which we potentially never requested.

Will fix.

> 
> > +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;
> > +
> > +	/* The MAC is capable of applying a delay on both Rx and Tx. Each
> > +	 * delay can either be on or off, there is no way to set its length.
> > +	 *
> > +	 * The exact delay applied depends on electric characteristics of the
> > +	 * board. The datasheet describes a typical Rx delay of 1800 ps and a
> > +	 * typical Tx delay of 2000 ps.
> > +	 *
> > +	 * There are boards where the RTSN device is used together with PHYs
> > +	 * who do not support a large enough internal delays to function. These
> > +	 * boards depends on the MAC applying these inexact delays.
> > +	 */
> > +
> > +	/* If the phy-mode is rgmii or rgmii-txid apply Rx delay on the MAC */
> > +	if (priv->iface == PHY_INTERFACE_MODE_RGMII ||
> > +	    priv->iface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +		if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay))
> > +			if (delay)
> > +				val |= GPOUT_RDM;
> > +
> > +	/* If the phy-mode is rgmii or rgmii-rxid apply Tx delay on the MAC */
> > +	if (priv->iface == PHY_INTERFACE_MODE_RGMII ||
> > +	    priv->iface == PHY_INTERFACE_MODE_RGMII_RXID)
> > +		if (!of_property_read_u32(np, "tx-internal-delay-ps", &delay))
> > +			if (delay)
> > +				val |= GPOUT_TDM;
> 
> This looks wrong. You should be applying delays for rgmii-id and
> rgmii-rxid. Plain rgmii means no delays are required, because the
> board has extra long clock lines. Same for TX delays, only for
> rgmii-tx or rgmii-id.

This confuses me a bit, from the bindings in ethernet-controller.yaml I 
get this the other way around,

      # RX and TX delays are added by the MAC when required
      - rgmii

      # RGMII with internal RX and TX delays provided by the PHY,
      # the MAC should not add the RX or TX delays in this case
      - rgmii-id

      # RGMII with internal RX delay provided by the PHY, the MAC
      # should not add an RX delay in this case
      - rgmii-rxid

      # RGMII with internal TX delay provided by the PHY, the MAC
      # should not add an TX delay in this case
      - rgmii-txid

The way I understand it is that if if the phy-mode is 'rgmii' the MAC 
shall apply delays if requested and only if the phy-mode is 'rgmii-id' 
shall the MAC completely ignore the delays and let the PHY handle it.

I might just be confused by the bindings. I will reverse the above for 
next version, but want to understand this correctly as this might need 
to be fixed in the other Renesas drivers as well.

> 
> > +static int rtsn_phy_init(struct rtsn_private *priv)
> > +{
> > +	struct device_node *np = priv->ndev->dev.parent->of_node;
> > +	struct phy_device *phydev;
> > +	struct device_node *phy;
> > +
> > +	priv->link = 0;
> > +
> > +	phy = of_parse_phandle(np, "phy-handle", 0);
> > +	if (!phy)
> > +		return -ENOENT;
> > +
> > +	phydev = of_phy_connect(priv->ndev, phy, rtsn_adjust_link, 0,
> > +				priv->iface);
> 
> This also looks wrong. Since you have applied the delays in the MAC,
> you need to mask the value passed to the PHY so it also does not apply
> delays.

Just so I understand correctly, if the phy-mode is A I should pass B to 
of_phy_connect() if I apply the delays in the MAC.

A               B
rgmii           rgmii-id
rgmii-id        rgmii
rgmii-rxid      rgmii-txid
rgmii-txid      rgmii-rxid

> 
>     Andrew
> 
> ---
> pw-bot: cr

-- 
Kind Regards,
Niklas Söderlund