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@xxxxxxxxxxxx>
> > +L: netdev@xxxxxxxxxxxxxxx
> > +L: linux-renesas-soc@xxxxxxxxxxxxxxx
> > +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@xxxxxxxxxxx>
> > L: alsa-devel@xxxxxxxxxxxxxxxx (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
