Received-SPF: pass (google.com: domain of linux-kernel+bounces-194955-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Ng, Boon Khai" <boon.khai.ng@intel.com>
To: Andrew Lunn <andrew@lunn.ch>
CC: Alexandre Torgue <alexandre.torgue@foss.st.com>, Jose Abreu
	<joabreu@synopsys.com>, "David S . Miller" <davem@davemloft.net>, "Eric
 Dumazet" <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-stm32@st-md-mailman.stormreply.com"
	<linux-stm32@st-md-mailman.stormreply.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "Ang, Tien Sung" <tien.sung.ang@intel.com>,
	"G Thomas, Rohan" <rohan.g.thomas@intel.com>, "Looi, Hong Aun"
	<hong.aun.looi@intel.com>, Andy Shevchenko
	<andriy.shevchenko@linux.intel.com>, Ilpo Jarvinen
	<ilpo.jarvinen@linux.intel.com>
Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net:
 stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
Thread-Topic: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net:
 stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
Thread-Index: AQHasBkLCj07Z540QUGXqLg1tlacPLGrb/8AgAB3OtCAALcdgIACos0A
Date: Thu, 30 May 2024 05:48:30 +0000
Message-ID: <DM8PR11MB5751194374C75EC5D5889D6AC1F32@DM8PR11MB5751.namprd11.prod.outlook.com>
References: <20240527093339.30883-1-boon.khai.ng@intel.com>
 <20240527093339.30883-2-boon.khai.ng@intel.com>
 <48176576-e1d2-4c45-967a-91cabb982a21@lunn.ch>
 <DM8PR11MB5751469FAA2B01EB6CEB7B50C1F12@DM8PR11MB5751.namprd11.prod.outlook.com>
 <48673551-cada-4194-865f-bc04c1e19c29@lunn.ch>
In-Reply-To: <48673551-cada-4194-865f-bc04c1e19c29@lunn.ch>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?bvkVTP6kFeUf5Gw0al+6A4daCC66Zs5AyalNzytwaMmriHt8M3bRxeiolrv7?=
 =?us-ascii?Q?8WcnWH5s/QNzuRpcHKlEcKcn9fZjZbf9EkKJMYM534YWmkGIOetQBltiP6sM?=
 =?us-ascii?Q?lUpQSiA24z+zcF5FoOt5FsCo1bxpzCktye2xueG7gAQbvsDiVzsLns41lF3y?=
 =?us-ascii?Q?Q+j82CPZbCwVHBkCbxcME7W8FIWe80uO9Y4krvDcebIi7uPv7o6OHLfmJFWc?=
 =?us-ascii?Q?Ep+ZV5UcpJF2ECTEC5AYUN0beYJqBZHTqEQ3uvY0FIpXiMp5kVcQkY69mkW4?=
 =?us-ascii?Q?ugaGKfQTpNJ77r4muxCUMtZL6PQlf/+O9eB8KsAG74w5VBGJ0gNLzJrt/gJS?=
 =?us-ascii?Q?XjjfBEMaCAu9gL1giZxUj9zP25eLlKCH80XKK3l9q38GrkrBKuLP4tr2E/47?=
 =?us-ascii?Q?xOGEj/R4Lf+bElpD4pTc3m27GkyKTcHykgqGQoAxjBa5kSQz7fiv/iYi8ae/?=
 =?us-ascii?Q?rOPjz7rk4uO/jACHdMgrumDLS23TzGQsf2aPrNJhespFrFRuj7kjgYk9bLp8?=
 =?us-ascii?Q?+Bd8QkFlfrNzY8V4jt7xGsSj0Gcj2FCRYsdsZh2bH6ulL5Kjh9ZYMYoBxxZN?=
 =?us-ascii?Q?0AJ0OoqsNSnggk0tPBphiMlNgkV26d+N10HDAjIYK6/SaERFp9d6Yos7I00a?=
 =?us-ascii?Q?s5uGMTDHx9rIaEO1drkdSbnFo9Egupme/6pNh1C3tBRoQ9WYREcTu7T/pNSZ?=
 =?us-ascii?Q?uS56khR+sPoIpPWzyz8zyjRE+lSJYVCqPMoNOX0nI9QGlYeJECQinZ8ZR/x3?=
 =?us-ascii?Q?k1uY2ZG0WH+Oyv6hLAXJTKvutE/xu165o9GzWk9YkrcC5HTpvlfG/ssKTvjc?=
 =?us-ascii?Q?iRh1K/ND1r/wg6B+n9cCxz9z/Ydcvx3PqdPuhaxUgsPRUjfiOzKzOLeRjhzh?=
 =?us-ascii?Q?RrKlSwhSCIARc46Gf6q+2RCBOae0XWmBEJy/5SeJygiUiyMVrtliUyXkV13e?=
 =?us-ascii?Q?NfF4JdFzQkC4z7PtIqIAnRwGFPWnbXSFgnVJIvY5vqLSOAHKQfMeLWXiZaO2?=
 =?us-ascii?Q?/vhHQi6YHwLTuBNci4QwW8EJ+gwyY8iZ4EYxuVd8uk4xDLcxyNlmVsFpucqh?=
 =?us-ascii?Q?R7yDX6xubcDHOygiJ4ezypavESy/k8CFQDznQD1uoGSO/YHRpeAJGq/1cFAw?=
 =?us-ascii?Q?4dNMgmm3vUQIKNVtXha8gsaHtxTvAtLps/hwvzxAj9johUu4SWyJAvysZTyT?=
 =?us-ascii?Q?Y8k1xI09+89WnAlveHLBYoUWzw/0eRTFDYg6uoIl5Kn0+WJCC6wz25D4nwlW?=
 =?us-ascii?Q?Vo6/93hopGlkc5H8ZZ1y100JbhCpKn2MIYTDKQt7jfL2HwIogNz3FskTBRvC?=
 =?us-ascii?Q?cYeifD+z/Q9FWnrDAsfgWPr2A6aKMQ+eEMhzbnQtfKYvRFd7NFXxmWkerj9i?=
 =?us-ascii?Q?0gD49EX/qdg2m+sRGSTXBycc8lJtLQ/9/qJQkXNtmVfKuFUqkuH9ePOTQg/D?=
 =?us-ascii?Q?BSZXACbWVy+DHQr0y5ENgDVBAMAcX5pLfIEOY72Lby9YowJ41h4PhOh+OikP?=
 =?us-ascii?Q?2T/mFECf3t29xrDlO4xOjeMvzeacPz0HalCRe2AJ1s99hPd8O7nBcqbpy2nV?=
 =?us-ascii?Q?bql5dltFCWtzdSZs/yseYlia+HN2q72avjPpmxXSsJBJPW0Ul2WMeSdQcEYz?=
 =?us-ascii?Q?ynZ41EKor9g4433ls+AXpPb6LaWd70J/bmERVO5CCxPP?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM8PR11MB5751.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f60a8003-973f-4e4d-1ea9-08dc806c22a8
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 May 2024 05:48:30.5931
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: tn391YElnsOYJCoXjeSjL1kNhf7JkVLeijB72vqHCEv+L2gU6ZblCHpodSBvQg70e0Oylycdbd9Y+xgfH90bpA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4991
X-OriginatorOrg: intel.com

>=20
> It is well know this driver is a mess. I just wanted to check you are not=
 adding
> to be mess by simply cut/pasting rather than refactoring code.
>

Okay sure Andrew, will take note on this.

=20
>=20
> static void dwmac4_rx_hw_vlan(struct mac_device_info *hw,
>                               struct dma_desc *rx_desc, struct sk_buff *s=
kb) {
>         if (hw->desc->get_rx_vlan_valid(rx_desc)) {
>                 u16 vid =3D hw->desc->get_rx_vlan_tci(rx_desc);
>=20
>                 __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
>         }
> }
>=20
> Looks identical to me.

Yes, some of the functions are identical, the driver has been divided=20
into dwmac4_core.c and dwxgmac2_core.c initially, so to enable the
rx_hw_vlan, it is porting from dwmac4_core to dwxgmac2_core.
 =20
> The basic flow is the same. Lets look at the #defines:
>=20
Right, the basic flow is direct copy and paste, and only the function
name is updated from dwmac4 to dwxgmac2.

> +#define XGMAC_VLAN_TAG_STRIP_NONE
> 	FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x0)
> +#define XGMAC_VLAN_TAG_STRIP_PASS
> 	FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x1)
> +#define XGMAC_VLAN_TAG_STRIP_FAIL
> 	FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x2)
> +#define XGMAC_VLAN_TAG_STRIP_ALL
> 	FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x3)
> #define GMAC_VLAN_TAG_STRIP_NONE        (0x0 <<
> GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
> #define GMAC_VLAN_TAG_STRIP_PASS        (0x1 <<
> GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
> #define GMAC_VLAN_TAG_STRIP_FAIL        (0x2 <<
> GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
> #define GMAC_VLAN_TAG_STRIP_ALL         (0x3 <<
> GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
>=20
> This is less obvious a straight cut/paste, but they are in fact identical=
.
>=20

This was Ilpo suggestion to use Field prep and Field get instead.

> #define GMAC_VLAN_TAG_CTRL_EVLRXS       BIT(24)
> #define XGMAC_VLAN_TAG_CTRL_EVLRXS	BIT(24)
>=20
> So this also looks identical to me, but maybe i'm missing something subtl=
e.
>=20

For VLAN register mapping, they don't have much different between
The dwmac4 and dwxgmac2, but the descriptor of getting the
VLAN id and VLAN packet is valid is a little bit different.

> +static inline bool dwxgmac2_wrback_get_rx_vlan_valid(struct dma_desc
> +*p) {
> +	u32 et_lt;
> +
> +	et_lt =3D FIELD_GET(XGMAC_RDES3_ET_LT, le32_to_cpu(p->des3));
> +
> +	return et_lt >=3D XGMAC_ET_LT_VLAN_STAG &&
> +	       et_lt <=3D XGMAC_ET_LT_DVLAN_STAG_CTAG; }
>=20
> static bool dwmac4_wrback_get_rx_vlan_valid(struct dma_desc *p) {
>         return ((le32_to_cpu(p->des3) & RDES3_LAST_DESCRIPTOR) &&
>                 (le32_to_cpu(p->des3) & RDES3_RDES0_VALID)); }
>=20
> #define RDES3_RDES0_VALID		BIT(25)
> #define RDES3_LAST_DESCRIPTOR		BIT(28)
>=20
> #define XGMAC_RDES3_ET_LT		GENMASK(19, 16)
> +#define XGMAC_ET_LT_VLAN_STAG		8
> +#define XGMAC_ET_LT_VLAN_CTAG		9
> +#define XGMAC_ET_LT_DVLAN_CTAG_CTAG	10
>=20
> This does actually look different.
>=20

Yes, this is the part in the descriptor where dwxgmac2 get the vlan  Valid.=
=20
it is described in Designware cores xgmac 10G Ethernet MAC version 3.10a
page 1573.

> Please take a step back and see if you can help clean up some of the mess=
 in
> this driver by refactoring bits of identical code, rather than copy/pasti=
ng it.
>=20

Appreciate if you could help to point out which part that I have to cleanup=
?
Do you mean I should combine the identical part between dwmac4_core.c
and dwxgmac2_core.c? or I should update the part that looks different and
make them the same?

Regards,
Boon Khai