Re: [PATCH] net: stmmac: dwxgmac2: enable L2 split header support

[Sagar Cheluvegowda <sagar.cheluvegowda@oss.qualcomm.com>]

Hi Lorenzo,

Thanks for the review.

> Hi Sagar,
>
> are we missing to set dma_cap->sphen for dwxgmac2?

No, that's already set — dwxgmac2_dma.c reads it from the HW_FEATURE1
register (dma_cap->sphen = (hw_cap & XGMAC_HWFEAT_SPHEN) >> 17;),
predates this patch and is unrelated to the sequence added here.

> +#define XGMAC_EXT_CFG0_VPRE		BIT(8)
> +#define XGMAC_EXT_CFG1			0x00000144
> +#define XGMAC_CONFIG1_SAVE_EN		BIT(24)
> +#define XGMAC_CONFIG1_SPLM(v)		FIELD_PREP(GENMASK(9, 8), v)
> I guess it would be better to define a mask for XGMAC_CONFIG1_SPLM.

Agreed, will send a v2 with:
    #define XGMAC_CONFIG1_SPLM_MASK        GENMASK(9, 8)
    #define XGMAC_CONFIG1_SPLM(v)        FIELD_PREP(XGMAC_CONFIG1_SPLM_MASK, v)

> +
> +	/* when FD=1 and LD=0, HL is RDES2[9:0] */
> +	if (!(rdes3 & XGMAC_RDES3_LD)) {
> +		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
> If XGMAC_RDES3_L34T is not defined on non-last descriptor, is it fine to use it
> here? IIUC XGMAC_RDES2_HL depends on XGMAC_RDES3_L34T, right?

The data book has the following table for this (Header Length
Availability in RDES2):
    Value of FD   Value of LD   Header Length (HL) Availability in RDES2
    0             1             HL not available
    1             0             HL is [9:0]
    1             1             - IP packets, HL is [9:0]
                                 - Non-IP packets, HL is [9:2]

So when FD=1,LD=0 the full RDES2[9:0] is valid regardless of packet
type; the L34T/L2T-gated split only applies when FD=1,LD=1
(single-descriptor packet). So the non-last-descriptor branch
deliberately doesn't consult L34T at all. This function is also only
ever called on the first descriptor (dwxgmac2_get_rx_header_len() is
invoked from stmmac_rx_buf1_len() only when len == 0), so that
precondition holds at the call site too.

Will send v2 with the SPLM mask fix.

>> +		return;
>> +	}
>> +
>> +	if (rdes3 & XGMAC_RDES3_L34T)
>>  		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
>> +	else if (rdes3 & XGMAC_RDES3_L2T)
>> +		*len = (le32_to_cpu(p->des2) & XGMAC_RDES2_NONIPHL) >> 2;
>>  }
>>  

Regards,
Sagar