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

[deepaksi <deepak.sikri@st.com>]

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