Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Hi Peppe,


Type 2 has been introduced starting from the 3.30a and Type 1 maintained
for back-ward compatibility because only available in 3.30.

If we want to actually support Type 1 (I've no HW where test) I guess we
need to review this patch.

First problem I can see in the patch is that, in case of type 1, we have
to properly set the device hw features because full IPC offload is not
supported (e.g. IPv6). This only is true for type 2.

I've just looked at all the MAC data-books starting from the 3.30a to
3.60a and I have seen that all the MACs treat the Receive Checksum
Offload Engine in the same way. I mean, the cores don't append any
payload csum bytes in case of type 2. This is always done for type 1!

Frankly, I prefer to have no define like GMAC_VERSION_35.
I always tried to avoid it :-)... unless there is some critical reason
and we actually need it. Pls, see my comments comments inline below.
There are two issues to be addresses.
1. Issue-1 : For the Type-1 Rx COE, the frame length has to be adjusted by 2. 2. The two frame status conditions that have been marked as csum_none for the versions 3.3a and
earlier.

I would take them along the review comments below



  	} else if (status == 0x1) {
  		CHIP_DBG(KERN_ERR
  		    "RX Des0 status: IPv4/6 unsupported IP PAYLOAD.\n");
-		ret = discard_frame;
+		ret = (mac_id>= GMAC_VERSION_35) ? discard_frame : csum_none;
  	} else if (status == 0x3) {
  		CHIP_DBG(KERN_ERR "RX Des0 status: No IPv4, IPv6 frame.\n");
-		ret = discard_frame;
+		ret = (mac_id>= GMAC_VERSION_35) ? discard_frame : csum_none;
  	}
  	return ret;
  }
The enh_desc_coe_rdes0 parses the Receive Descriptor 0 When COE (Type
2). It should be onlyinvoked on full csum case.
We also should discard the frames on the latest two cases I mean when:

- IPv4/IPv6 Type frame with no IP header checksum error and the payload
check bypassed, due to an unsupported payload

- A Type frame that is neither IPv4 or IPv6 (the Checksum Offload engine
bypasses checksum completely.)

Also from all the Synopsys databooks I cannot see any difference in the
Table 7.2 when treat the RDES0 bits 0, 5, 7.

So I expect to have no check for the GMAC_VERSION_35 inside the enh desc
file.

I can cross verify this on the SPEAr test platform which we had been using. We had faced some issue
related to this before also.

  static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
-				  struct dma_desc *p)
+				  struct dma_desc *p, u32 mac_id)
  {
  	int ret = good_frame;
  	struct net_device_stats *stats = (struct net_device_stats *)data;
@@ -195,9 +198,11 @@ static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
  	/* After a payload csum error, the ES bit is set.
  	 * It doesn't match with the information reported into the databook.
  	 * At any rate, we need to understand if the CSUM hw computation is ok
-	 * and report this info to the upper layers. */
+	 * and report this info to the upper layers.
+	 */
This is useless change in the patch that should be removed in the final
version.

ok

  	if (priv->rx_coe)
  		pr_info(" Checksum Offload Engine supported\n");
+
do not add useless spaces.

ok

  	if (priv->plat->tx_coe)
  		pr_info(" Checksum insertion supported\n");
Here I expect to see more information about the RX COE ;-)

I'd like to see:
   pr_info(" Checksum Offload Engine supported (type %d)\n", ....);

Sure, will do that


@@ -1436,7 +1437,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)

  		/* read the status of the incoming frame */
  		status = (priv->hw->desc->rx_status(&priv->dev->stats,
-						&priv->xstats, p));
+					&priv->xstats, p,
+					priv->hw->synopsys_uid&  0xff));
  		if (unlikely(status == discard_frame))
  			priv->dev->stats.rx_errors++;
  		else {
@@ -1444,6 +1446,16 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
  			int frame_len;

  			frame_len = priv->hw->desc->get_rx_frame_len(p);
+			/*
+			 * The type-1 checksume offload engines append
+			 * the checksum at the end of frame, and the
+			 * the two bytes of checksum are added in
+			 * length.
+			 * Adjust for that in the framelen for type-1
+			 * checksum offload engines.
+			 */
+			if (priv->plat->csum_off_engine_type == STMMAC_CSUM_T1)
+				frame_len -= 2;
I'd like to have this inside the core. I mean, get_rx_frame_len returns
the len - 2 in case of type 1.

Thats fine. Will do that

Regards
Deepak
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux Kernel Discussion]     [Ethernet Bridging]     [Linux Wireless Networking]     [Linux Bluetooth Networking]     [Linux Networking Users]     [VLAN]     [Git]     [IETF Annouce]     [Linux Assembly]     [Security]     [Bugtraq]     [Photo]     [Singles Social Networking]     [Yosemite Information]     [MIPS Linux]     [ARM Linux Kernel]     [ARM Linux]     [Linux Virtualization]     [Linux Security]     [Linux IDE]     [Linux RAID]     [Linux SCSI]     [Free Dating]

Add to Google Powered by Linux