RE: [PATCH net-next 08/18] ravb: Add R-Car common features

Hi Sergei,

Thanks for the feedback.

Subject: Re: [PATCH net-next 08/18] ravb: Add R-Car common features

Hello!

On 7/22/21 5:13 PM, Biju Das wrote:

quoted

The below features are supported by both R-Car Gen2 and Gen3.

1) magic packet detection
2) multiple TSRQ support
3) extended descriptor in rx

   I think this one should better be called timestamping...

OK. Will change it to timestamp.

quoted

4) No half duplex support

   Couldn't we avoid the "negative" features?

10 Mbps/Half duplex(As per the comment below) is not supported in R-Car. May be it should be a "positive" feature for
RZ/G2L. probably we need to add this as part of RZ/G2L feature.

quoted

5) override mtu change

   Hm, I'd vote for the individual patches covering only single feature...

OK. That will make patches again simpler.

quoted

Add features bits to support the same.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 110
+++++++++++++++--------
 1 file changed, 71 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c

b/drivers/net/ethernet/renesas/ravb_main.c
index b3c99f974632..4ef2565534d2 100644

--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

[...]

quoted

@@ -680,11 +694,14 @@ static void ravb_rcv_snd_enable(struct

net_device *ndev)
 /* function for waiting dma process finished */  static int
ravb_stop_dma(struct net_device *ndev)  {
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	int error;

 	/* Wait for stopping the hardware TX process */
-	error = ravb_wait(ndev, TCCR,
-			  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
+	if (info->features & RAVB_MULTI_TSRQ)
+		error = ravb_wait(ndev, TCCR,
+				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 |

TCCR_TSRQ3, 0);

quoted

 	if (error)

   What if the above *if* skips the ravb_wait() call -- didn't you get a
complaint from gcc about the unnintialized variable?

Good catch will initialize with error = 0 and later patch when we add RZ/G2L, will remove this unnecessary initialization.

[...]

quoted

@@ -808,11 +826,14 @@ static bool ravb_queue_interrupt(struct

net_device *ndev, int q)

 static bool ravb_timestamp_interrupt(struct net_device *ndev)  {
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	u32 tis = ravb_read(ndev, TIS);

 	if (tis & TIS_TFUF) {
 		ravb_write(ndev, ~(TIS_TFUF | TIS_RESERVED), TIS);
-		ravb_get_tx_tstamp(ndev);
+		if (info->features & RAVB_EX_RX_DESC)

   Yeah, definitely a bad feature name...

OK. AS you suggested, will change the macro RAVB_EX_RX_DESC to ravb_timestamp feature bit like sh_eth.

quoted

+			ravb_get_tx_tstamp(ndev);
 		return true;
 	}
 	return false;

[...]

quoted

@@ -1069,15 +1091,17 @@ static int ravb_phy_init(struct net_device

*ndev)

quoted

 		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
 	}

-	/* 10BASE, Pause and Asym Pause is not supported */
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
+	if (info->features & RAVB_NO_HALF_DUPLEX) {
+		/* 10BASE, Pause and Asym Pause is not supported */
+		phy_remove_link_mode(phydev,

ETHTOOL_LINK_MODE_10baseT_Half_BIT);

quoted

+		phy_remove_link_mode(phydev,

ETHTOOL_LINK_MODE_10baseT_Full_BIT);

quoted

+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
+		phy_remove_link_mode(phydev,

ETHTOOL_LINK_MODE_Asym_Pause_BIT);

quoted

-	/* Half Duplex is not supported */
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+		/* Half Duplex is not supported */
+		phy_remove_link_mode(phydev,

ETHTOOL_LINK_MODE_1000baseT_Half_BIT);

quoted

+		phy_remove_link_mode(phydev,

ETHTOOL_LINK_MODE_100baseT_Half_BIT);

    Mhm? Some of the half-duplex modes sre unsupported still?

[...]

quoted

@@ -1314,8 +1338,9 @@ static void ravb_get_wol(struct net_device

*ndev, struct ethtool_wolinfo *wol)  static int ravb_set_wol(struct
net_device *ndev, struct ethtool_wolinfo *wol)  {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;

-	if (wol->wolopts & ~WAKE_MAGIC)
+	if ((wol->wolopts & ~WAKE_MAGIC) || (!(info->features &
+RAVB_MAGIC)))

   Parens about !x not needed. And I think the second operand should come
first instead...

OK.

quoted

 		return -EOPNOTSUPP;

 	priv->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);

[...]

quoted

@@ -1595,28 +1621,30 @@ static netdev_tx_t ravb_start_xmit(struct

sk_buff *skb, struct net_device *ndev)

quoted

 	desc->dptr = cpu_to_le32(dma_addr);

 	/* TX timestamp required */
-	if (q == RAVB_NC) {
-		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
-		if (!ts_skb) {
-			if (num_tx_desc > 1) {
-				desc--;
-				dma_unmap_single(ndev->dev.parent, dma_addr,
-						 len, DMA_TO_DEVICE);
+	if (info->features & RAVB_EX_RX_DESC) {

   Definitely a bad name...

OK.

[...]

quoted

@@ -2205,8 +2235,10 @@ static int ravb_probe(struct platform_device

*pdev)

quoted

 	}
 	clk_prepare_enable(priv->refclk);

-	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
-	ndev->min_mtu = ETH_MIN_MTU;
+	if (info->features & RAVB_OVERRIDE_MTU_CHANGE) {

   Why? :-/ Could you tell me more details?

RX buff size = 2048 for R-Car where as it is 8K for RZ/G2L.
So Do you think we should use the same max_mtu for RZ/G2L as well?

Please let us know your recommendation here in this case?  The original bsp code used default "eth_mtu_change"
which is now deprecated.

Regards,
Biju

quoted

+		ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
+		ndev->min_mtu = ETH_MIN_MTU;
+	}

 	priv->num_tx_desc = info->num_tx_desc;

MBR, Sergei