[PATCH net-next 00/13] net: fec: cleanups, update quirk, update IRQ naming

[PATCH net-next 00/13] net: fec: cleanups, update quirk, update IRQ naming

[Marc Kleine-Budde]

This series first cleans up the fec driver a bit (typos, obsolete
comments, add missing header files, rename struct, replace magic
number by defines).

The next 2 patches update the order of IRQs in the driver and gives
them names that reflect their function.

The last 5 patches clean up the fec_enet_rx_queue() function,
including VLAN handling.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (13):
      net: fec: fix typos found by codespell
      net: fec: struct fec_enet_private: remove obsolete comment
      net: fec: add missing header files
      net: fec: rename struct fec_devinfo fec_imx6x_info -> fec_imx6sx_info
      net: fec: fec_restart(): introduce a define for FEC_ECR_SPEED
      net: fec: fec_restart(): make use of FEC_ECR_RESET
      net: fec: fec_probe(): update quirk: bring IRQs in correct order
      net: fec: fec_probe(): let IRQ name reflect its function
      net: fec: fec_enet_rx_queue(): use same signature as fec_enet_tx_queue()
      net: fec: fec_enet_rx_queue(): replace open coded cast by skb_vlan_eth_hdr()
      net: fec: fec_enet_rx_queue(): reduce scope of data
      net: fec: fec_enet_rx_queue(): move_call to _vlan_hwaccel_put_tag()
      net: fec: fec_enet_rx_queue(): factor out VLAN handling into separate function fec_enet_rx_vlan()

 drivers/net/ethernet/freescale/fec.h      | 35 ++++++++++---
 drivers/net/ethernet/freescale/fec_main.c | 85 +++++++++++++++++--------------
 drivers/net/ethernet/freescale/fec_ptp.c  |  9 ++--
 3 files changed, 81 insertions(+), 48 deletions(-)
---
base-commit: 53bac8330865417332d4cf80cb671b15956b049d
change-id: 20240925-fec-cleanups-c9b5544bc854

Best regards,
-- 
Marc Kleine-Budde <mkl@pengutronix.de>

[PATCH net-next 01/13] net: fec: fix typos found by codespell

[Marc Kleine-Budde]

codespell has found some typos in the comments, fix them.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.h     | 8 ++++----
 drivers/net/ethernet/freescale/fec_ptp.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 1cca0425d49397bbdb97f2c058bd759f9e602f17..77c2a08d23542accdb85b37a6f86847d9eb56a7a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -115,7 +115,7 @@
 #define IEEE_T_MCOL		0x254 /* Frames tx'd with multiple collision */
 #define IEEE_T_DEF		0x258 /* Frames tx'd after deferral delay */
 #define IEEE_T_LCOL		0x25c /* Frames tx'd with late collision */
-#define IEEE_T_EXCOL		0x260 /* Frames tx'd with excesv collisions */
+#define IEEE_T_EXCOL		0x260 /* Frames tx'd with excessive collisions */
 #define IEEE_T_MACERR		0x264 /* Frames tx'd with TX FIFO underrun */
 #define IEEE_T_CSERR		0x268 /* Frames tx'd with carrier sense err */
 #define IEEE_T_SQE		0x26c /* Frames tx'd with SQE err */
@@ -342,7 +342,7 @@ struct bufdesc_ex {
 #define FEC_TX_BD_FTYPE(X)	(((X) & 0xf) << 20)
 
 /* The number of Tx and Rx buffers.  These are allocated from the page
- * pool.  The code may assume these are power of two, so it it best
+ * pool.  The code may assume these are power of two, so it is best
  * to keep them that size.
  * We don't need to allocate pages for the transmitter.  We just use
  * the skbuffer directly.
@@ -460,7 +460,7 @@ struct bufdesc_ex {
 #define FEC_QUIRK_SINGLE_MDIO		(1 << 11)
 /* Controller supports RACC register */
 #define FEC_QUIRK_HAS_RACC		(1 << 12)
-/* Controller supports interrupt coalesc */
+/* Controller supports interrupt coalesce */
 #define FEC_QUIRK_HAS_COALESCE		(1 << 13)
 /* Interrupt doesn't wake CPU from deep idle */
 #define FEC_QUIRK_ERR006687		(1 << 14)
@@ -495,7 +495,7 @@ struct bufdesc_ex {
  */
 #define FEC_QUIRK_HAS_EEE		(1 << 20)
 
-/* i.MX8QM ENET IP version add new feture to generate delayed TXC/RXC
+/* i.MX8QM ENET IP version add new feature to generate delayed TXC/RXC
  * as an alternative option to make sure it works well with various PHYs.
  * For the implementation of delayed clock, ENET takes synchronized 250MHz
  * clocks to generate 2ns delay.
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 7f6b57432071667e8553363f7c8c21198f38f530..8722f623d9e47e385439f1cee8c677e2b95b236d 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -118,7 +118,7 @@ static u64 fec_ptp_read(const struct cyclecounter *cc)
  * @fep: the fec_enet_private structure handle
  * @enable: enable the channel pps output
  *
- * This function enble the PPS ouput on the timer channel.
+ * This function enable the PPS output on the timer channel.
  */
 static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
 {
@@ -173,7 +173,7 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
 		 * very close to the second point, which means NSEC_PER_SEC
 		 * - ts.tv_nsec is close to be zero(For example 20ns); Since the timer
 		 * is still running when we calculate the first compare event, it is
-		 * possible that the remaining nanoseonds run out before the compare
+		 * possible that the remaining nanoseconds run out before the compare
 		 * counter is calculated and written into TCCR register. To avoid
 		 * this possibility, we will set the compare event to be the next
 		 * of next second. The current setting is 31-bit timer and wrap

-- 
2.45.2

RE: [PATCH net-next 01/13] net: fec: fix typos found by codespell

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 01/13] net: fec: fix typos found by codespell
> 
> codespell has found some typos in the comments, fix them.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec.h     | 8 ++++----
>  drivers/net/ethernet/freescale/fec_ptp.c | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index
> 1cca0425d49397bbdb97f2c058bd759f9e602f17..77c2a08d23542accdb85b37
> a6f86847d9eb56a7a 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -115,7 +115,7 @@
>  #define IEEE_T_MCOL		0x254 /* Frames tx'd with multiple collision */
>  #define IEEE_T_DEF		0x258 /* Frames tx'd after deferral delay */
>  #define IEEE_T_LCOL		0x25c /* Frames tx'd with late collision */
> -#define IEEE_T_EXCOL		0x260 /* Frames tx'd with excesv collisions */
> +#define IEEE_T_EXCOL		0x260 /* Frames tx'd with excessive collisions */
>  #define IEEE_T_MACERR		0x264 /* Frames tx'd with TX FIFO underrun
> */
>  #define IEEE_T_CSERR		0x268 /* Frames tx'd with carrier sense err */
>  #define IEEE_T_SQE		0x26c /* Frames tx'd with SQE err */
> @@ -342,7 +342,7 @@ struct bufdesc_ex {
>  #define FEC_TX_BD_FTYPE(X)	(((X) & 0xf) << 20)
> 
>  /* The number of Tx and Rx buffers.  These are allocated from the page
> - * pool.  The code may assume these are power of two, so it it best
> + * pool.  The code may assume these are power of two, so it is best
>   * to keep them that size.
>   * We don't need to allocate pages for the transmitter.  We just use
>   * the skbuffer directly.
> @@ -460,7 +460,7 @@ struct bufdesc_ex {
>  #define FEC_QUIRK_SINGLE_MDIO		(1 << 11)
>  /* Controller supports RACC register */
>  #define FEC_QUIRK_HAS_RACC		(1 << 12)
> -/* Controller supports interrupt coalesc */
> +/* Controller supports interrupt coalesce */
>  #define FEC_QUIRK_HAS_COALESCE		(1 << 13)
>  /* Interrupt doesn't wake CPU from deep idle */
>  #define FEC_QUIRK_ERR006687		(1 << 14)
> @@ -495,7 +495,7 @@ struct bufdesc_ex {
>   */
>  #define FEC_QUIRK_HAS_EEE		(1 << 20)
> 
> -/* i.MX8QM ENET IP version add new feture to generate delayed TXC/RXC
> +/* i.MX8QM ENET IP version add new feature to generate delayed TXC/RXC
>   * as an alternative option to make sure it works well with various PHYs.
>   * For the implementation of delayed clock, ENET takes synchronized
> 250MHz
>   * clocks to generate 2ns delay.
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index
> 7f6b57432071667e8553363f7c8c21198f38f530..8722f623d9e47e385439f1c
> ee8c677e2b95b236d 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -118,7 +118,7 @@ static u64 fec_ptp_read(const struct cyclecounter *cc)
>   * @fep: the fec_enet_private structure handle
>   * @enable: enable the channel pps output
>   *
> - * This function enble the PPS ouput on the timer channel.
> + * This function enable the PPS output on the timer channel.
>   */
>  static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
> { @@ -173,7 +173,7 @@ static int fec_ptp_enable_pps(struct
> fec_enet_private *fep, uint enable)
>  		 * very close to the second point, which means NSEC_PER_SEC
>  		 * - ts.tv_nsec is close to be zero(For example 20ns); Since the timer
>  		 * is still running when we calculate the first compare event, it is
> -		 * possible that the remaining nanoseonds run out before the
> compare
> +		 * possible that the remaining nanoseconds run out before the
> compare
>  		 * counter is calculated and written into TCCR register. To avoid
>  		 * this possibility, we will set the compare event to be the next
>  		 * of next second. The current setting is 31-bit timer and wrap
> 
> --
> 2.45.2
> 

It looks good to me, thanks.

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH net-next 01/13] net: fec: fix typos found by codespell

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:49PM +0200, Marc Kleine-Budde wrote:
> codespell has found some typos in the comments, fix them.
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/net/ethernet/freescale/fec.h     | 8 ++++----
>  drivers/net/ethernet/freescale/fec_ptp.c | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 1cca0425d49397bbdb97f2c058bd759f9e602f17..77c2a08d23542accdb85b37a6f86847d9eb56a7a 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -115,7 +115,7 @@
>  #define IEEE_T_MCOL		0x254 /* Frames tx'd with multiple collision */
>  #define IEEE_T_DEF		0x258 /* Frames tx'd after deferral delay */
>  #define IEEE_T_LCOL		0x25c /* Frames tx'd with late collision */
> -#define IEEE_T_EXCOL		0x260 /* Frames tx'd with excesv collisions */
> +#define IEEE_T_EXCOL		0x260 /* Frames tx'd with excessive collisions */
>  #define IEEE_T_MACERR		0x264 /* Frames tx'd with TX FIFO underrun */
>  #define IEEE_T_CSERR		0x268 /* Frames tx'd with carrier sense err */
>  #define IEEE_T_SQE		0x26c /* Frames tx'd with SQE err */
> @@ -342,7 +342,7 @@ struct bufdesc_ex {
>  #define FEC_TX_BD_FTYPE(X)	(((X) & 0xf) << 20)
>
>  /* The number of Tx and Rx buffers.  These are allocated from the page
> - * pool.  The code may assume these are power of two, so it it best
> + * pool.  The code may assume these are power of two, so it is best
>   * to keep them that size.
>   * We don't need to allocate pages for the transmitter.  We just use
>   * the skbuffer directly.
> @@ -460,7 +460,7 @@ struct bufdesc_ex {
>  #define FEC_QUIRK_SINGLE_MDIO		(1 << 11)
>  /* Controller supports RACC register */
>  #define FEC_QUIRK_HAS_RACC		(1 << 12)
> -/* Controller supports interrupt coalesc */
> +/* Controller supports interrupt coalesce */
>  #define FEC_QUIRK_HAS_COALESCE		(1 << 13)
>  /* Interrupt doesn't wake CPU from deep idle */
>  #define FEC_QUIRK_ERR006687		(1 << 14)
> @@ -495,7 +495,7 @@ struct bufdesc_ex {
>   */
>  #define FEC_QUIRK_HAS_EEE		(1 << 20)
>
> -/* i.MX8QM ENET IP version add new feture to generate delayed TXC/RXC
> +/* i.MX8QM ENET IP version add new feature to generate delayed TXC/RXC
>   * as an alternative option to make sure it works well with various PHYs.
>   * For the implementation of delayed clock, ENET takes synchronized 250MHz
>   * clocks to generate 2ns delay.
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 7f6b57432071667e8553363f7c8c21198f38f530..8722f623d9e47e385439f1cee8c677e2b95b236d 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -118,7 +118,7 @@ static u64 fec_ptp_read(const struct cyclecounter *cc)
>   * @fep: the fec_enet_private structure handle
>   * @enable: enable the channel pps output
>   *
> - * This function enble the PPS ouput on the timer channel.
> + * This function enable the PPS output on the timer channel.
>   */
>  static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
>  {
> @@ -173,7 +173,7 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
>  		 * very close to the second point, which means NSEC_PER_SEC
>  		 * - ts.tv_nsec is close to be zero(For example 20ns); Since the timer
>  		 * is still running when we calculate the first compare event, it is
> -		 * possible that the remaining nanoseonds run out before the compare
> +		 * possible that the remaining nanoseconds run out before the compare
>  		 * counter is calculated and written into TCCR register. To avoid
>  		 * this possibility, we will set the compare event to be the next
>  		 * of next second. The current setting is 31-bit timer and wrap
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 01/13] net: fec: fix typos found by codespell

[Csókás Bence]

Hi,

On 2024. 10. 16. 23:51, Marc Kleine-Budde wrote:
> codespell has found some typos in the comments, fix them.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>   drivers/net/ethernet/freescale/fec.h     | 8 ++++----
>   drivers/net/ethernet/freescale/fec_ptp.c | 4 ++--
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 1cca0425d49397bbdb97f2c058bd759f9e602f17..77c2a08d23542accdb85b37a6f86847d9eb56a7a 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -115,7 +115,7 @@
>   #define IEEE_T_MCOL		0x254 /* Frames tx'd with multiple collision */
>   #define IEEE_T_DEF		0x258 /* Frames tx'd after deferral delay */
>   #define IEEE_T_LCOL		0x25c /* Frames tx'd with late collision */
> -#define IEEE_T_EXCOL		0x260 /* Frames tx'd with excesv collisions */
> +#define IEEE_T_EXCOL		0x260 /* Frames tx'd with excessive collisions */
>   #define IEEE_T_MACERR		0x264 /* Frames tx'd with TX FIFO underrun */
>   #define IEEE_T_CSERR		0x268 /* Frames tx'd with carrier sense err */
>   #define IEEE_T_SQE		0x26c /* Frames tx'd with SQE err */
> @@ -342,7 +342,7 @@ struct bufdesc_ex {
>   #define FEC_TX_BD_FTYPE(X)	(((X) & 0xf) << 20)
>   
>   /* The number of Tx and Rx buffers.  These are allocated from the page
> - * pool.  The code may assume these are power of two, so it it best
> + * pool.  The code may assume these are power of two, so it is best
>    * to keep them that size.
>    * We don't need to allocate pages for the transmitter.  We just use
>    * the skbuffer directly.
> @@ -460,7 +460,7 @@ struct bufdesc_ex {
>   #define FEC_QUIRK_SINGLE_MDIO		(1 << 11)
>   /* Controller supports RACC register */
>   #define FEC_QUIRK_HAS_RACC		(1 << 12)
> -/* Controller supports interrupt coalesc */
> +/* Controller supports interrupt coalesce */
>   #define FEC_QUIRK_HAS_COALESCE		(1 << 13)
>   /* Interrupt doesn't wake CPU from deep idle */
>   #define FEC_QUIRK_ERR006687		(1 << 14)
> @@ -495,7 +495,7 @@ struct bufdesc_ex {
>    */
>   #define FEC_QUIRK_HAS_EEE		(1 << 20)
>   
> -/* i.MX8QM ENET IP version add new feture to generate delayed TXC/RXC
> +/* i.MX8QM ENET IP version add new feature to generate delayed TXC/RXC
>    * as an alternative option to make sure it works well with various PHYs.
>    * For the implementation of delayed clock, ENET takes synchronized 250MHz
>    * clocks to generate 2ns delay.
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 7f6b57432071667e8553363f7c8c21198f38f530..8722f623d9e47e385439f1cee8c677e2b95b236d 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -118,7 +118,7 @@ static u64 fec_ptp_read(const struct cyclecounter *cc)
>    * @fep: the fec_enet_private structure handle
>    * @enable: enable the channel pps output
>    *
> - * This function enble the PPS ouput on the timer channel.
> + * This function enable the PPS output on the timer channel.

enableS

>    */
>   static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
>   {
> @@ -173,7 +173,7 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
>   		 * very close to the second point, which means NSEC_PER_SEC
>   		 * - ts.tv_nsec is close to be zero(For example 20ns); Since the timer
>   		 * is still running when we calculate the first compare event, it is
> -		 * possible that the remaining nanoseonds run out before the compare
> +		 * possible that the remaining nanoseconds run out before the compare
>   		 * counter is calculated and written into TCCR register. To avoid
>   		 * this possibility, we will set the compare event to be the next
>   		 * of next second. The current setting is 31-bit timer and wrap
> 

Otherwise, LGTM.
Reviewed-by: Csókás, Bence <csokas.bence@prolan.hu>

Bence

[PATCH net-next 02/13] net: fec: struct fec_enet_private: remove obsolete comment

[Marc Kleine-Budde]

In commit 4d494cdc92b3 ("net: fec: change data structure to support
multiqueue") the data structures were changed, so that the comment
about the sent-in-place skb doesn't apply any more. Remove it.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 77c2a08d23542accdb85b37a6f86847d9eb56a7a..e55c7ccad2ec39a9f3492135675d480a22f7032d 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -614,7 +614,6 @@ struct fec_enet_private {
 	unsigned int num_tx_queues;
 	unsigned int num_rx_queues;
 
-	/* The saved address of a sent-in-place packet/buffer, for skfree(). */
 	struct fec_enet_priv_tx_q *tx_queue[FEC_ENET_MAX_TX_QS];
 	struct fec_enet_priv_rx_q *rx_queue[FEC_ENET_MAX_RX_QS];
 

-- 
2.45.2

RE: [PATCH net-next 02/13] net: fec: struct fec_enet_private: remove obsolete comment

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 02/13] net: fec: struct fec_enet_private: remove
> obsolete comment
> 
> In commit 4d494cdc92b3 ("net: fec: change data structure to support
> multiqueue") the data structures were changed, so that the comment
> about the sent-in-place skb doesn't apply any more. Remove it.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index
> 77c2a08d23542accdb85b37a6f86847d9eb56a7a..e55c7ccad2ec39a9f349213
> 5675d480a22f7032d 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -614,7 +614,6 @@ struct fec_enet_private {
>  	unsigned int num_tx_queues;
>  	unsigned int num_rx_queues;
> 
> -	/* The saved address of a sent-in-place packet/buffer, for skfree(). */
>  	struct fec_enet_priv_tx_q *tx_queue[FEC_ENET_MAX_TX_QS];
>  	struct fec_enet_priv_rx_q *rx_queue[FEC_ENET_MAX_RX_QS];
> 
> 
> --
> 2.45.2
> 

Thanks.

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH net-next 02/13] net: fec: struct fec_enet_private: remove obsolete comment

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:50PM +0200, Marc Kleine-Budde wrote:
> In commit 4d494cdc92b3 ("net: fec: change data structure to support
> multiqueue") the data structures were changed, so that the comment
> about the sent-in-place skb doesn't apply any more. Remove it.

nit: wrap at 75 char

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 77c2a08d23542accdb85b37a6f86847d9eb56a7a..e55c7ccad2ec39a9f3492135675d480a22f7032d 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -614,7 +614,6 @@ struct fec_enet_private {
>  	unsigned int num_tx_queues;
>  	unsigned int num_rx_queues;
>
> -	/* The saved address of a sent-in-place packet/buffer, for skfree(). */
>  	struct fec_enet_priv_tx_q *tx_queue[FEC_ENET_MAX_TX_QS];
>  	struct fec_enet_priv_rx_q *rx_queue[FEC_ENET_MAX_RX_QS];
>
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 02/13] net: fec: struct fec_enet_private: remove obsolete comment

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On 16.10.2024 21:55:09, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:51:50PM +0200, Marc Kleine-Budde wrote:
> > In commit 4d494cdc92b3 ("net: fec: change data structure to support
> > multiqueue") the data structures were changed, so that the comment
> > about the sent-in-place skb doesn't apply any more. Remove it.
> 
> nit: wrap at 75 char

fixed - my editor is set to auto-wrap at 70.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 03/13] net: fec: add missing header files

[Marc Kleine-Budde]

The fec.h isn't self contained. Add missing header files, so that it
can be parsed by language servers without errors.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index e55c7ccad2ec39a9f3492135675d480a22f7032d..63744a86752540fcede7fc4c29865b2529492526 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -15,7 +15,9 @@
 /****************************************************************************/
 
 #include <linux/clocksource.h>
+#include <linux/ethtool.h>
 #include <linux/net_tstamp.h>
+#include <linux/phy.h>
 #include <linux/pm_qos.h>
 #include <linux/bpf.h>
 #include <linux/ptp_clock_kernel.h>

-- 
2.45.2

RE: [PATCH net-next 03/13] net: fec: add missing header files

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 03/13] net: fec: add missing header files
> 
> The fec.h isn't self contained. Add missing header files, so that it
> can be parsed by language servers without errors.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index
> e55c7ccad2ec39a9f3492135675d480a22f7032d..63744a86752540fcede7fc4c
> 29865b2529492526 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -15,7 +15,9 @@
> 
> /******************************************************************
> **********/
> 
>  #include <linux/clocksource.h>
> +#include <linux/ethtool.h>
>  #include <linux/net_tstamp.h>
> +#include <linux/phy.h>
>  #include <linux/pm_qos.h>
>  #include <linux/bpf.h>
>  #include <linux/ptp_clock_kernel.h>
> 
> --
> 2.45.2
> 

Thanks.

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH net-next 03/13] net: fec: add missing header files

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:51PM +0200, Marc Kleine-Budde wrote:
> The fec.h isn't self contained. Add missing header files, so that it
> can be parsed by language servers without errors.

nit: wrap at 75 char

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index e55c7ccad2ec39a9f3492135675d480a22f7032d..63744a86752540fcede7fc4c29865b2529492526 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -15,7 +15,9 @@
>  /****************************************************************************/
>
>  #include <linux/clocksource.h>
> +#include <linux/ethtool.h>
>  #include <linux/net_tstamp.h>
> +#include <linux/phy.h>
>  #include <linux/pm_qos.h>
>  #include <linux/bpf.h>
>  #include <linux/ptp_clock_kernel.h>
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 03/13] net: fec: add missing header files

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On 16.10.2024 21:56:13, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:51:51PM +0200, Marc Kleine-Budde wrote:
> > The fec.h isn't self contained. Add missing header files, so that it
> > can be parsed by language servers without errors.
> 
> nit: wrap at 75 char

Fixed,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 04/13] net: fec: rename struct fec_devinfo fec_imx6x_info -> fec_imx6sx_info

[Marc Kleine-Budde]

In da722186f654 ("net: fec: set GPR bit on suspend by DT
configuration.") the platform_device_id fec_devtype::driver_data was
converted from holding the quirks to a pointing to struct fec_devinfo.

The struct fec_devinfo holding the information for the i.MX6SX was
named fec_imx6x_info.

To align the name of the struct with the SoC's name, rename struct
fec_devinfo fec_imx6x_info to struct fec_devinfo fec_imx6sx_info.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1b55047c0237cbea4e44a5a8335af5c11e2325f8..c57039cc83228dcd980a8fdbc18cd3eab2dfe1a5 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -131,7 +131,7 @@ static const struct fec_devinfo fec_mvf600_info = {
 		  FEC_QUIRK_HAS_MDIO_C45,
 };
 
-static const struct fec_devinfo fec_imx6x_info = {
+static const struct fec_devinfo fec_imx6sx_info = {
 	.quirks = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
 		  FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
 		  FEC_QUIRK_HAS_VLAN | FEC_QUIRK_HAS_AVB |
@@ -196,7 +196,7 @@ static const struct of_device_id fec_dt_ids[] = {
 	{ .compatible = "fsl,imx28-fec", .data = &fec_imx28_info, },
 	{ .compatible = "fsl,imx6q-fec", .data = &fec_imx6q_info, },
 	{ .compatible = "fsl,mvf600-fec", .data = &fec_mvf600_info, },
-	{ .compatible = "fsl,imx6sx-fec", .data = &fec_imx6x_info, },
+	{ .compatible = "fsl,imx6sx-fec", .data = &fec_imx6sx_info, },
 	{ .compatible = "fsl,imx6ul-fec", .data = &fec_imx6ul_info, },
 	{ .compatible = "fsl,imx8mq-fec", .data = &fec_imx8mq_info, },
 	{ .compatible = "fsl,imx8qm-fec", .data = &fec_imx8qm_info, },

-- 
2.45.2

RE: [PATCH net-next 04/13] net: fec: rename struct fec_devinfo fec_imx6x_info -> fec_imx6sx_info

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 04/13] net: fec: rename struct fec_devinfo
> fec_imx6x_info -> fec_imx6sx_info
> 
> In da722186f654 ("net: fec: set GPR bit on suspend by DT
> configuration.") the platform_device_id fec_devtype::driver_data was
> converted from holding the quirks to a pointing to struct fec_devinfo.
> 
> The struct fec_devinfo holding the information for the i.MX6SX was
> named fec_imx6x_info.
> 
> To align the name of the struct with the SoC's name, rename struct
> fec_devinfo fec_imx6x_info to struct fec_devinfo fec_imx6sx_info.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> 1b55047c0237cbea4e44a5a8335af5c11e2325f8..c57039cc83228dcd980a8fd
> bc18cd3eab2dfe1a5 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -131,7 +131,7 @@ static const struct fec_devinfo fec_mvf600_info = {
>  		  FEC_QUIRK_HAS_MDIO_C45,
>  };
> 
> -static const struct fec_devinfo fec_imx6x_info = {
> +static const struct fec_devinfo fec_imx6sx_info = {
>  	.quirks = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
>  		  FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
>  		  FEC_QUIRK_HAS_VLAN | FEC_QUIRK_HAS_AVB |
> @@ -196,7 +196,7 @@ static const struct of_device_id fec_dt_ids[] = {
>  	{ .compatible = "fsl,imx28-fec", .data = &fec_imx28_info, },
>  	{ .compatible = "fsl,imx6q-fec", .data = &fec_imx6q_info, },
>  	{ .compatible = "fsl,mvf600-fec", .data = &fec_mvf600_info, },
> -	{ .compatible = "fsl,imx6sx-fec", .data = &fec_imx6x_info, },
> +	{ .compatible = "fsl,imx6sx-fec", .data = &fec_imx6sx_info, },
>  	{ .compatible = "fsl,imx6ul-fec", .data = &fec_imx6ul_info, },
>  	{ .compatible = "fsl,imx8mq-fec", .data = &fec_imx8mq_info, },
>  	{ .compatible = "fsl,imx8qm-fec", .data = &fec_imx8qm_info, },
> 
> --
> 2.45.2
> 

Thanks.

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH net-next 04/13] net: fec: rename struct fec_devinfo fec_imx6x_info -> fec_imx6sx_info

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:52PM +0200, Marc Kleine-Budde wrote:
> In da722186f654 ("net: fec: set GPR bit on suspend by DT
> configuration.") the platform_device_id fec_devtype::driver_data was
> converted from holding the quirks to a pointing to struct fec_devinfo.
>
> The struct fec_devinfo holding the information for the i.MX6SX was
> named fec_imx6x_info.
>
> To align the name of the struct with the SoC's name, rename struct
> fec_devinfo fec_imx6x_info to struct fec_devinfo fec_imx6sx_info.

Is it better
"Rename fec_imx6x_info to fec_imx6sx_info to align SoC's name."

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 1b55047c0237cbea4e44a5a8335af5c11e2325f8..c57039cc83228dcd980a8fdbc18cd3eab2dfe1a5 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -131,7 +131,7 @@ static const struct fec_devinfo fec_mvf600_info = {
>  		  FEC_QUIRK_HAS_MDIO_C45,
>  };
>
> -static const struct fec_devinfo fec_imx6x_info = {
> +static const struct fec_devinfo fec_imx6sx_info = {
>  	.quirks = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
>  		  FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
>  		  FEC_QUIRK_HAS_VLAN | FEC_QUIRK_HAS_AVB |
> @@ -196,7 +196,7 @@ static const struct of_device_id fec_dt_ids[] = {
>  	{ .compatible = "fsl,imx28-fec", .data = &fec_imx28_info, },
>  	{ .compatible = "fsl,imx6q-fec", .data = &fec_imx6q_info, },
>  	{ .compatible = "fsl,mvf600-fec", .data = &fec_mvf600_info, },
> -	{ .compatible = "fsl,imx6sx-fec", .data = &fec_imx6x_info, },
> +	{ .compatible = "fsl,imx6sx-fec", .data = &fec_imx6sx_info, },
>  	{ .compatible = "fsl,imx6ul-fec", .data = &fec_imx6ul_info, },
>  	{ .compatible = "fsl,imx8mq-fec", .data = &fec_imx8mq_info, },
>  	{ .compatible = "fsl,imx8qm-fec", .data = &fec_imx8qm_info, },
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 04/13] net: fec: rename struct fec_devinfo fec_imx6x_info -> fec_imx6sx_info

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On 16.10.2024 21:59:59, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:51:52PM +0200, Marc Kleine-Budde wrote:
> > In da722186f654 ("net: fec: set GPR bit on suspend by DT
> > configuration.") the platform_device_id fec_devtype::driver_data was
> > converted from holding the quirks to a pointing to struct fec_devinfo.
> >
> > The struct fec_devinfo holding the information for the i.MX6SX was
> > named fec_imx6x_info.
> >
> > To align the name of the struct with the SoC's name, rename struct
> > fec_devinfo fec_imx6x_info to struct fec_devinfo fec_imx6sx_info.
> 
> Is it better
> "Rename fec_imx6x_info to fec_imx6sx_info to align SoC's name."

Fixed,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 05/13] net: fec: fec_restart(): introduce a define for FEC_ECR_SPEED

[Marc Kleine-Budde]

Instead of open coding the bit mask to configure for 1000 MBit/s add a
define for it.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c57039cc83228dcd980a8fdbc18cd3eab2dfe1a5..2ee7e4765ba3163fb0d158e60b534b171da26c22 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -276,6 +276,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define FEC_ECR_MAGICEN         BIT(2)
 #define FEC_ECR_SLEEP           BIT(3)
 #define FEC_ECR_EN1588          BIT(4)
+#define FEC_ECR_SPEED           BIT(5)
 #define FEC_ECR_BYTESWP         BIT(8)
 /* FEC RCR bits definition */
 #define FEC_RCR_LOOP            BIT(0)
@@ -1160,7 +1161,7 @@ fec_restart(struct net_device *ndev)
 		/* 1G, 100M or 10M */
 		if (ndev->phydev) {
 			if (ndev->phydev->speed == SPEED_1000)
-				ecntl |= (1 << 5);
+				ecntl |= FEC_ECR_SPEED;
 			else if (ndev->phydev->speed == SPEED_100)
 				rcntl &= ~FEC_RCR_10BASET;
 			else

-- 
2.45.2

RE: [PATCH net-next 05/13] net: fec: fec_restart(): introduce a define for FEC_ECR_SPEED

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 05/13] net: fec: fec_restart(): introduce a define for
> FEC_ECR_SPEED
> 
> Instead of open coding the bit mask to configure for 1000 MBit/s add a define
> for it.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> c57039cc83228dcd980a8fdbc18cd3eab2dfe1a5..2ee7e4765ba3163fb0d158e
> 60b534b171da26c22 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -276,6 +276,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC
> address");
>  #define FEC_ECR_MAGICEN         BIT(2)
>  #define FEC_ECR_SLEEP           BIT(3)
>  #define FEC_ECR_EN1588          BIT(4)
> +#define FEC_ECR_SPEED           BIT(5)
>  #define FEC_ECR_BYTESWP         BIT(8)
>  /* FEC RCR bits definition */
>  #define FEC_RCR_LOOP            BIT(0)
> @@ -1160,7 +1161,7 @@ fec_restart(struct net_device *ndev)
>  		/* 1G, 100M or 10M */
>  		if (ndev->phydev) {
>  			if (ndev->phydev->speed == SPEED_1000)
> -				ecntl |= (1 << 5);
> +				ecntl |= FEC_ECR_SPEED;
>  			else if (ndev->phydev->speed == SPEED_100)
>  				rcntl &= ~FEC_RCR_10BASET;
>  			else
> 
> --
> 2.45.2
> 

Thanks

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH net-next 05/13] net: fec: fec_restart(): introduce a define for FEC_ECR_SPEED

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:53PM +0200, Marc Kleine-Budde wrote:
> Instead of open coding the bit mask to configure for 1000 MBit/s add a
> define for it.

Replace "1 << 5" for configuring 1000 MBit/s with a defined constant to
improve code readability and maintainability.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index c57039cc83228dcd980a8fdbc18cd3eab2dfe1a5..2ee7e4765ba3163fb0d158e60b534b171da26c22 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -276,6 +276,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>  #define FEC_ECR_MAGICEN         BIT(2)
>  #define FEC_ECR_SLEEP           BIT(3)
>  #define FEC_ECR_EN1588          BIT(4)
> +#define FEC_ECR_SPEED           BIT(5)
>  #define FEC_ECR_BYTESWP         BIT(8)
>  /* FEC RCR bits definition */
>  #define FEC_RCR_LOOP            BIT(0)
> @@ -1160,7 +1161,7 @@ fec_restart(struct net_device *ndev)
>  		/* 1G, 100M or 10M */
>  		if (ndev->phydev) {
>  			if (ndev->phydev->speed == SPEED_1000)
> -				ecntl |= (1 << 5);
> +				ecntl |= FEC_ECR_SPEED;
>  			else if (ndev->phydev->speed == SPEED_100)
>  				rcntl &= ~FEC_RCR_10BASET;
>  			else
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 05/13] net: fec: fec_restart(): introduce a define for FEC_ECR_SPEED

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

On 16.10.2024 22:05:36, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:51:53PM +0200, Marc Kleine-Budde wrote:
> > Instead of open coding the bit mask to configure for 1000 MBit/s add a
> > define for it.
> 
> Replace "1 << 5" for configuring 1000 MBit/s with a defined constant to
> improve code readability and maintainability.

Fixed,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 06/13] net: fec: fec_restart(): make use of FEC_ECR_RESET

[Marc Kleine-Budde]

Replace the magic number "1" by the already existing define
FEC_ECR_RESET.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 2ee7e4765ba3163fb0d158e60b534b171da26c22..d948ed9810027d5fabe521dc3af2cf505dacd13e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1089,7 +1089,7 @@ fec_restart(struct net_device *ndev)
 	    ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) {
 		writel(0, fep->hwp + FEC_ECNTRL);
 	} else {
-		writel(1, fep->hwp + FEC_ECNTRL);
+		writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL);
 		udelay(10);
 	}
 

-- 
2.45.2

RE: [PATCH net-next 06/13] net: fec: fec_restart(): make use of FEC_ECR_RESET

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 06/13] net: fec: fec_restart(): make use of
> FEC_ECR_RESET
> 
> Replace the magic number "1" by the already existing define
> FEC_ECR_RESET.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> 2ee7e4765ba3163fb0d158e60b534b171da26c22..d948ed9810027d5fabe521
> dc3af2cf505dacd13e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1089,7 +1089,7 @@ fec_restart(struct net_device *ndev)
>  	    ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) {
>  		writel(0, fep->hwp + FEC_ECNTRL);
>  	} else {
> -		writel(1, fep->hwp + FEC_ECNTRL);
> +		writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL);
>  		udelay(10);
>  	}
> 
> 
> --
> 2.45.2
> 

Thanks.

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH net-next 06/13] net: fec: fec_restart(): make use of FEC_ECR_RESET

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:54PM +0200, Marc Kleine-Budde wrote:
> Replace the magic number "1" by the already existing define
> FEC_ECR_RESET.

nit: wrap at 75 char.
Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 2ee7e4765ba3163fb0d158e60b534b171da26c22..d948ed9810027d5fabe521dc3af2cf505dacd13e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1089,7 +1089,7 @@ fec_restart(struct net_device *ndev)
>  	    ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) {
>  		writel(0, fep->hwp + FEC_ECNTRL);
>  	} else {
> -		writel(1, fep->hwp + FEC_ECNTRL);
> +		writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL);
>  		udelay(10);
>  	}
>
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 06/13] net: fec: fec_restart(): make use of FEC_ECR_RESET

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On 16.10.2024 22:07:04, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:51:54PM +0200, Marc Kleine-Budde wrote:
> > Replace the magic number "1" by the already existing define
> > FEC_ECR_RESET.
> 
> nit: wrap at 75 char.
> Reviewed-by: Frank Li <Frank.Li@nxp.com>

Fixed,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Marc Kleine-Budde]

With i.MX8MQ and compatible SoCs, the order of the IRQs in the device
tree is not optimal. The driver expects the first three IRQs to match
their corresponding queue, while the last (fourth) IRQ is used for the
PPS:

- 1st IRQ: "int0": queue0 + other IRQs
- 2nd IRQ: "int1": queue1
- 3rd IRQ: "int2": queue2
- 4th IRQ: "pps": pps

However, the i.MX8MQ and compatible SoCs do not use the
"interrupt-names" property and specify the IRQs in the wrong order:

- 1st IRQ: queue1
- 2nd IRQ: queue2
- 3rd IRQ: queue0 + other IRQs
- 4th IRQ: pps

First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to
FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality.

If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back
in the correct order, this is done in fec_probe().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.h      | 24 ++++++++++++++++++++++--
 drivers/net/ethernet/freescale/fec_main.c | 18 +++++++++++-------
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 63744a86752540fcede7fc4c29865b2529492526..b0f1a3e28d5c8052be3a8a0fa18303a1df2bb5bd 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -504,8 +504,28 @@ struct bufdesc_ex {
  */
 #define FEC_QUIRK_DELAYED_CLKS_SUPPORT	(1 << 21)
 
-/* i.MX8MQ SoC integration mix wakeup interrupt signal into "int2" interrupt line. */
-#define FEC_QUIRK_WAKEUP_FROM_INT2	(1 << 22)
+/* With i.MX8MQ and compatible SoCs, the order of the IRQs in the
+ * device tree is not optimal. The driver expects the first three IRQs
+ * to match their corresponding queue, while the last (fourth) IRQ is
+ * used for the PPS:
+ *
+ * - 1st IRQ: "int0": queue0 + other IRQs
+ * - 2nd IRQ: "int1": queue1
+ * - 3rd IRQ: "int2": queue2
+ * - 4th IRQ: "pps": pps
+ *
+ * However, the i.MX8MQ and compatible SoCs do not use the
+ * "interrupt-names" property and specify the IRQs in the wrong order:
+ *
+ * - 1st IRQ: queue1
+ * - 2nd IRQ: queue2
+ * - 3rd IRQ: queue0 + other IRQs
+ * - 4th IRQ: pps
+ *
+ * If the following quirk is active, put the IRQs back in the correct
+ * order, this is done in fec_probe().
+ */
+#define FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ	BIT(22)
 
 /* i.MX6Q adds pm_qos support */
 #define FEC_QUIRK_HAS_PMQOS			BIT(23)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d948ed9810027d5fabe521dc3af2cf505dacd13e..f124ffe3619d82dc089f8494d33d2398e6f631fb 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -157,7 +157,7 @@ static const struct fec_devinfo fec_imx8mq_info = {
 		  FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE |
 		  FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE |
 		  FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES |
-		  FEC_QUIRK_HAS_EEE | FEC_QUIRK_WAKEUP_FROM_INT2 |
+		  FEC_QUIRK_HAS_EEE | FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ |
 		  FEC_QUIRK_HAS_MDIO_C45,
 };
 
@@ -4260,10 +4260,7 @@ static void fec_enet_get_wakeup_irq(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
-	if (fep->quirks & FEC_QUIRK_WAKEUP_FROM_INT2)
-		fep->wake_irq = fep->irq[2];
-	else
-		fep->wake_irq = fep->irq[0];
+	fep->wake_irq = fep->irq[0];
 }
 
 static int fec_enet_init_stop_mode(struct fec_enet_private *fep,
@@ -4495,10 +4492,17 @@ fec_probe(struct platform_device *pdev)
 		goto failed_init;
 
 	for (i = 0; i < irq_cnt; i++) {
-		snprintf(irq_name, sizeof(irq_name), "int%d", i);
+		int irq_num;
+
+		if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ)
+			irq_num = (i + irq_cnt - 1) % irq_cnt;
+		else
+			irq_num = i;
+
+		snprintf(irq_name, sizeof(irq_name), "int%d", irq_num);
 		irq = platform_get_irq_byname_optional(pdev, irq_name);
 		if (irq < 0)
-			irq = platform_get_irq(pdev, i);
+			irq = platform_get_irq(pdev, irq_num);
 		if (irq < 0) {
 			ret = irq;
 			goto failed_irq;

-- 
2.45.2

Re: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:55PM +0200, Marc Kleine-Budde wrote:
> With i.MX8MQ and compatible SoCs, the order of the IRQs in the device
> tree is not optimal. The driver expects the first three IRQs to match
> their corresponding queue, while the last (fourth) IRQ is used for the
> PPS:
>
> - 1st IRQ: "int0": queue0 + other IRQs
> - 2nd IRQ: "int1": queue1
> - 3rd IRQ: "int2": queue2
> - 4th IRQ: "pps": pps
>
> However, the i.MX8MQ and compatible SoCs do not use the
> "interrupt-names" property and specify the IRQs in the wrong order:
>
> - 1st IRQ: queue1
> - 2nd IRQ: queue2
> - 3rd IRQ: queue0 + other IRQs
> - 4th IRQ: pps

why not fix dts?

Frank

>
> First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to
> FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality.
>
> If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back
> in the correct order, this is done in fec_probe().
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec.h      | 24 ++++++++++++++++++++++--
>  drivers/net/ethernet/freescale/fec_main.c | 18 +++++++++++-------
>  2 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 63744a86752540fcede7fc4c29865b2529492526..b0f1a3e28d5c8052be3a8a0fa18303a1df2bb5bd 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -504,8 +504,28 @@ struct bufdesc_ex {
>   */
>  #define FEC_QUIRK_DELAYED_CLKS_SUPPORT	(1 << 21)
>
> -/* i.MX8MQ SoC integration mix wakeup interrupt signal into "int2" interrupt line. */
> -#define FEC_QUIRK_WAKEUP_FROM_INT2	(1 << 22)
> +/* With i.MX8MQ and compatible SoCs, the order of the IRQs in the
> + * device tree is not optimal. The driver expects the first three IRQs
> + * to match their corresponding queue, while the last (fourth) IRQ is
> + * used for the PPS:
> + *
> + * - 1st IRQ: "int0": queue0 + other IRQs
> + * - 2nd IRQ: "int1": queue1
> + * - 3rd IRQ: "int2": queue2
> + * - 4th IRQ: "pps": pps
> + *
> + * However, the i.MX8MQ and compatible SoCs do not use the
> + * "interrupt-names" property and specify the IRQs in the wrong order:
> + *
> + * - 1st IRQ: queue1
> + * - 2nd IRQ: queue2
> + * - 3rd IRQ: queue0 + other IRQs
> + * - 4th IRQ: pps
> + *
> + * If the following quirk is active, put the IRQs back in the correct
> + * order, this is done in fec_probe().
> + */
> +#define FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ	BIT(22)
>
>  /* i.MX6Q adds pm_qos support */
>  #define FEC_QUIRK_HAS_PMQOS			BIT(23)
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index d948ed9810027d5fabe521dc3af2cf505dacd13e..f124ffe3619d82dc089f8494d33d2398e6f631fb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -157,7 +157,7 @@ static const struct fec_devinfo fec_imx8mq_info = {
>  		  FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE |
>  		  FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE |
>  		  FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES |
> -		  FEC_QUIRK_HAS_EEE | FEC_QUIRK_WAKEUP_FROM_INT2 |
> +		  FEC_QUIRK_HAS_EEE | FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ |
>  		  FEC_QUIRK_HAS_MDIO_C45,
>  };
>
> @@ -4260,10 +4260,7 @@ static void fec_enet_get_wakeup_irq(struct platform_device *pdev)
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>
> -	if (fep->quirks & FEC_QUIRK_WAKEUP_FROM_INT2)
> -		fep->wake_irq = fep->irq[2];
> -	else
> -		fep->wake_irq = fep->irq[0];
> +	fep->wake_irq = fep->irq[0];
>  }
>
>  static int fec_enet_init_stop_mode(struct fec_enet_private *fep,
> @@ -4495,10 +4492,17 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_init;
>
>  	for (i = 0; i < irq_cnt; i++) {
> -		snprintf(irq_name, sizeof(irq_name), "int%d", i);
> +		int irq_num;
> +
> +		if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ)
> +			irq_num = (i + irq_cnt - 1) % irq_cnt;
> +		else
> +			irq_num = i;
> +
> +		snprintf(irq_name, sizeof(irq_name), "int%d", irq_num);
>  		irq = platform_get_irq_byname_optional(pdev, irq_name);
>  		if (irq < 0)
> -			irq = platform_get_irq(pdev, i);
> +			irq = platform_get_irq(pdev, irq_num);
>  		if (irq < 0) {
>  			ret = irq;
>  			goto failed_irq;
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1401 bytes --]

On 16.10.2024 22:15:43, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:51:55PM +0200, Marc Kleine-Budde wrote:
> > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device
> > tree is not optimal. The driver expects the first three IRQs to match
> > their corresponding queue, while the last (fourth) IRQ is used for the
> > PPS:
> >
> > - 1st IRQ: "int0": queue0 + other IRQs
> > - 2nd IRQ: "int1": queue1
> > - 3rd IRQ: "int2": queue2
> > - 4th IRQ: "pps": pps
> >
> > However, the i.MX8MQ and compatible SoCs do not use the
> > "interrupt-names" property and specify the IRQs in the wrong order:
> >
> > - 1st IRQ: queue1
> > - 2nd IRQ: queue2
> > - 3rd IRQ: queue0 + other IRQs
> > - 4th IRQ: pps
> 
> why not fix dts?

Because of compatibility. You could update the fec driver and try to
detect if the IRQs in the DTS are in the "correct" order, but the new
DTS will be incompatible with the old driver.

I'm working on a patch series that updates the fec driver to "per queue
IRQ and NAPI" handling. For this approach it's crucial that the IRQs
match the queue number.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs
> in correct order
> 
> With i.MX8MQ and compatible SoCs, the order of the IRQs in the device
> tree is not optimal. The driver expects the first three IRQs to match
> their corresponding queue, while the last (fourth) IRQ is used for the
> PPS:
> 
> - 1st IRQ: "int0": queue0 + other IRQs
> - 2nd IRQ: "int1": queue1
> - 3rd IRQ: "int2": queue2
> - 4th IRQ: "pps": pps
> 
> However, the i.MX8MQ and compatible SoCs do not use the
> "interrupt-names" property and specify the IRQs in the wrong order:
> 
> - 1st IRQ: queue1
> - 2nd IRQ: queue2
> - 3rd IRQ: queue0 + other IRQs
> - 4th IRQ: pps
> 
> First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to
> FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality.
> 
> If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back
> in the correct order, this is done in fec_probe().
> 

I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2
is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM and
its compatible SoCs, but i.MX8QM uses a different solution. I don't know why
there are two different ways of doing it, as I don't know the history. But you can
refer to the solution of i.MX8QM, which I think is more suitable.

See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is
placed first.

fec1: ethernet@5b040000 {
		reg = <0x5b040000 0x10000>;
		interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>;

> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec.h      | 24 ++++++++++++++++++++++--
>  drivers/net/ethernet/freescale/fec_main.c | 18 +++++++++++-------
>  2 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index
> 63744a86752540fcede7fc4c29865b2529492526..b0f1a3e28d5c8052be3a8a0
> fa18303a1df2bb5bd 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -504,8 +504,28 @@ struct bufdesc_ex {
>   */
>  #define FEC_QUIRK_DELAYED_CLKS_SUPPORT	(1 << 21)
> 
> -/* i.MX8MQ SoC integration mix wakeup interrupt signal into "int2" interrupt
> line. */
> -#define FEC_QUIRK_WAKEUP_FROM_INT2	(1 << 22)
> +/* With i.MX8MQ and compatible SoCs, the order of the IRQs in the
> + * device tree is not optimal. The driver expects the first three IRQs
> + * to match their corresponding queue, while the last (fourth) IRQ is
> + * used for the PPS:
> + *
> + * - 1st IRQ: "int0": queue0 + other IRQs
> + * - 2nd IRQ: "int1": queue1
> + * - 3rd IRQ: "int2": queue2
> + * - 4th IRQ: "pps": pps
> + *
> + * However, the i.MX8MQ and compatible SoCs do not use the
> + * "interrupt-names" property and specify the IRQs in the wrong order:
> + *
> + * - 1st IRQ: queue1
> + * - 2nd IRQ: queue2
> + * - 3rd IRQ: queue0 + other IRQs
> + * - 4th IRQ: pps
> + *
> + * If the following quirk is active, put the IRQs back in the correct
> + * order, this is done in fec_probe().
> + */
> +#define FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ	BIT(22)
> 
>  /* i.MX6Q adds pm_qos support */
>  #define FEC_QUIRK_HAS_PMQOS			BIT(23)
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> d948ed9810027d5fabe521dc3af2cf505dacd13e..f124ffe3619d82dc089f8494
> d33d2398e6f631fb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -157,7 +157,7 @@ static const struct fec_devinfo fec_imx8mq_info = {
>  		  FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE |
>  		  FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE |
>  		  FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES
> |
> -		  FEC_QUIRK_HAS_EEE | FEC_QUIRK_WAKEUP_FROM_INT2 |
> +		  FEC_QUIRK_HAS_EEE | FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ |
>  		  FEC_QUIRK_HAS_MDIO_C45,
>  };
> 
> @@ -4260,10 +4260,7 @@ static void fec_enet_get_wakeup_irq(struct
> platform_device *pdev)
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> 
> -	if (fep->quirks & FEC_QUIRK_WAKEUP_FROM_INT2)
> -		fep->wake_irq = fep->irq[2];
> -	else
> -		fep->wake_irq = fep->irq[0];
> +	fep->wake_irq = fep->irq[0];
>  }
> 
>  static int fec_enet_init_stop_mode(struct fec_enet_private *fep,
> @@ -4495,10 +4492,17 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_init;
> 
>  	for (i = 0; i < irq_cnt; i++) {
> -		snprintf(irq_name, sizeof(irq_name), "int%d", i);
> +		int irq_num;
> +
> +		if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ)
> +			irq_num = (i + irq_cnt - 1) % irq_cnt;
> +		else
> +			irq_num = i;
> +
> +		snprintf(irq_name, sizeof(irq_name), "int%d", irq_num);
>  		irq = platform_get_irq_byname_optional(pdev, irq_name);
>  		if (irq < 0)
> -			irq = platform_get_irq(pdev, i);
> +			irq = platform_get_irq(pdev, irq_num);
>  		if (irq < 0) {
>  			ret = irq;
>  			goto failed_irq;
> 
> --
> 2.45.2
> 

Re: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

On 17.10.2024 03:09:15, Wei Fang wrote:
> > -----Original Message-----
> > From: Marc Kleine-Budde <mkl@pengutronix.de>
> > Sent: 2024年10月17日 5:52
> > To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> > Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> > Cochran <richardcochran@gmail.com>
> > Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs
> > in correct order
> > 
> > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device
> > tree is not optimal. The driver expects the first three IRQs to match
> > their corresponding queue, while the last (fourth) IRQ is used for the
> > PPS:
> > 
> > - 1st IRQ: "int0": queue0 + other IRQs
> > - 2nd IRQ: "int1": queue1
> > - 3rd IRQ: "int2": queue2
> > - 4th IRQ: "pps": pps
> > 
> > However, the i.MX8MQ and compatible SoCs do not use the
> > "interrupt-names" property and specify the IRQs in the wrong order:
> > 
> > - 1st IRQ: queue1
> > - 2nd IRQ: queue2
> > - 3rd IRQ: queue0 + other IRQs
> > - 4th IRQ: pps
> > 
> > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to
> > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality.
> > 
> > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back
> > in the correct order, this is done in fec_probe().
> > 
> 
> I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2
> is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM and
> its compatible SoCs, but i.MX8QM uses a different solution. I don't know why
> there are two different ways of doing it, as I don't know the history. But you can
> refer to the solution of i.MX8QM, which I think is more suitable.
> 
> See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is
> placed first.

Yes, that is IMHO the correct description of the IP core, but the
i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
reasons (fixed DTS with old driver) it's IMHO not possible to change the
DTS.

> fec1: ethernet@5b040000 {
> 		reg = <0x5b040000 0x10000>;
> 		interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>;

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Wei Fang]

> > > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring
> IRQs
> > > in correct order
> > >
> > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device
> > > tree is not optimal. The driver expects the first three IRQs to match
> > > their corresponding queue, while the last (fourth) IRQ is used for the
> > > PPS:
> > >
> > > - 1st IRQ: "int0": queue0 + other IRQs
> > > - 2nd IRQ: "int1": queue1
> > > - 3rd IRQ: "int2": queue2
> > > - 4th IRQ: "pps": pps
> > >
> > > However, the i.MX8MQ and compatible SoCs do not use the
> > > "interrupt-names" property and specify the IRQs in the wrong order:
> > >
> > > - 1st IRQ: queue1
> > > - 2nd IRQ: queue2
> > > - 3rd IRQ: queue0 + other IRQs
> > > - 4th IRQ: pps
> > >
> > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to
> > > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality.
> > >
> > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back
> > > in the correct order, this is done in fec_probe().
> > >
> >
> > I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2
> > is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM
> and
> > its compatible SoCs, but i.MX8QM uses a different solution. I don't know
> why
> > there are two different ways of doing it, as I don't know the history. But you
> can
> > refer to the solution of i.MX8QM, which I think is more suitable.
> >
> > See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is
> > placed first.
> 
> Yes, that is IMHO the correct description of the IP core, but the
> i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> reasons (fixed DTS with old driver) it's IMHO not possible to change the
> DTS.
> 

I don't think it is a correct behavior for old drivers to use new DTBs or new
drivers to use old DTBs. Maybe you are correct, Frank also asked the same
question, let's see how Frank responded.

> > fec1: ethernet@5b040000 {
> > 		reg = <0x5b040000 0x10000>;
> > 		interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>,
> > 			     <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>,
> > 			     <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>,
> > 			     <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>;
> 

Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2468 bytes --]

On 17.10.2024 07:43:55, Wei Fang wrote:
> > > > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring
> > IRQs
> > > > in correct order
> > > >
> > > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device
> > > > tree is not optimal. The driver expects the first three IRQs to match
> > > > their corresponding queue, while the last (fourth) IRQ is used for the
> > > > PPS:
> > > >
> > > > - 1st IRQ: "int0": queue0 + other IRQs
> > > > - 2nd IRQ: "int1": queue1
> > > > - 3rd IRQ: "int2": queue2
> > > > - 4th IRQ: "pps": pps
> > > >
> > > > However, the i.MX8MQ and compatible SoCs do not use the
> > > > "interrupt-names" property and specify the IRQs in the wrong order:
> > > >
> > > > - 1st IRQ: queue1
> > > > - 2nd IRQ: queue2
> > > > - 3rd IRQ: queue0 + other IRQs
> > > > - 4th IRQ: pps
> > > >
> > > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to
> > > > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality.
> > > >
> > > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back
> > > > in the correct order, this is done in fec_probe().
> > > >
> > >
> > > I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2
> > > is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM
> > and
> > > its compatible SoCs, but i.MX8QM uses a different solution. I don't know
> > why
> > > there are two different ways of doing it, as I don't know the history. But you
> > can
> > > refer to the solution of i.MX8QM, which I think is more suitable.
> > >
> > > See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is
> > > placed first.
> > 
> > Yes, that is IMHO the correct description of the IP core, but the
> > i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> > reasons (fixed DTS with old driver) it's IMHO not possible to change the
> > DTS.
> > 
> 
> I don't think it is a correct behavior for old drivers to use new DTBs or new
> drivers to use old DTBs. Maybe you are correct, Frank also asked the same
> question, let's see how Frank responded.

DTBs should be considered stable ABI.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Frank Li]

On Thu, Oct 17, 2024 at 12:21:10PM +0200, Marc Kleine-Budde wrote:
> On 17.10.2024 07:43:55, Wei Fang wrote:
> > > > > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring
> > > IRQs
> > > > > in correct order
> > > > >
> > > > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device
> > > > > tree is not optimal. The driver expects the first three IRQs to match
> > > > > their corresponding queue, while the last (fourth) IRQ is used for the
> > > > > PPS:
> > > > >
> > > > > - 1st IRQ: "int0": queue0 + other IRQs
> > > > > - 2nd IRQ: "int1": queue1
> > > > > - 3rd IRQ: "int2": queue2
> > > > > - 4th IRQ: "pps": pps
> > > > >
> > > > > However, the i.MX8MQ and compatible SoCs do not use the
> > > > > "interrupt-names" property and specify the IRQs in the wrong order:
> > > > >
> > > > > - 1st IRQ: queue1
> > > > > - 2nd IRQ: queue2
> > > > > - 3rd IRQ: queue0 + other IRQs
> > > > > - 4th IRQ: pps
> > > > >
> > > > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to
> > > > > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality.
> > > > >
> > > > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back
> > > > > in the correct order, this is done in fec_probe().
> > > > >
> > > >
> > > > I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2
> > > > is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM
> > > and
> > > > its compatible SoCs, but i.MX8QM uses a different solution. I don't know
> > > why
> > > > there are two different ways of doing it, as I don't know the history. But you
> > > can
> > > > refer to the solution of i.MX8QM, which I think is more suitable.
> > > >
> > > > See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is
> > > > placed first.
> > >
> > > Yes, that is IMHO the correct description of the IP core, but the
> > > i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> > > reasons (fixed DTS with old driver) it's IMHO not possible to change the
> > > DTS.
> > >
> >
> > I don't think it is a correct behavior for old drivers to use new DTBs or new
> > drivers to use old DTBs. Maybe you are correct, Frank also asked the same
> > question, let's see how Frank responded.
>
> DTBs should be considered stable ABI.
>

ABI defined at binding doc.
  interrupt-names:
    oneOf:
      - items:
          - const: int0
      - items:
          - const: int0
          - const: pps
      - items:
          - const: int0
          - const: int1
          - const: int2
      - items:
          - const: int0
          - const: int1
          - const: int2
          - const: pps

DTB should align binding doc. There are not 'descriptions' at 'interrupt',
which should match 'interrupt-names'. So IMX8MP dts have not match ABI,
which defined by binding doc. So it is DTS implement wrong.

Frank

> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

On 17.10.2024 10:03:51, Frank Li wrote:
> > > > Yes, that is IMHO the correct description of the IP core, but the
> > > > i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> > > > reasons (fixed DTS with old driver) it's IMHO not possible to change the
> > > > DTS.
> > > >
> > >
> > > I don't think it is a correct behavior for old drivers to use new DTBs or new
> > > drivers to use old DTBs. Maybe you are correct, Frank also asked the same
> > > question, let's see how Frank responded.
> >
> > DTBs should be considered stable ABI.
> >
> 
> ABI defined at binding doc.
>   interrupt-names:
>     oneOf:
>       - items:
>           - const: int0
>       - items:
>           - const: int0
>           - const: pps
>       - items:
>           - const: int0
>           - const: int1
>           - const: int2
>       - items:
>           - const: int0
>           - const: int1
>           - const: int2
>           - const: pps
> 
> DTB should align binding doc. There are not 'descriptions' at 'interrupt',
> which should match 'interrupt-names'. So IMX8MP dts have not match ABI,
> which defined by binding doc. So it is DTS implement wrong.

I follow your conclusion. But keep in mind, fixing the DTB would break
compatibility. The wrong DTS looks like this:

- const: int1
- const: int2
- const: int0
- const: pps

Currently we have broken DTS on the i.MX8M* and the
FEC_QUIRK_WAKEUP_FROM_INT2 that "fixes" this.

This patch uses this quirk to correct the IRQ <-> queue assignment in
the driver.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Frank Li]

On Thu, Oct 17, 2024 at 04:21:33PM +0200, Marc Kleine-Budde wrote:
> On 17.10.2024 10:03:51, Frank Li wrote:
> > > > > Yes, that is IMHO the correct description of the IP core, but the
> > > > > i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> > > > > reasons (fixed DTS with old driver) it's IMHO not possible to change the
> > > > > DTS.
> > > > >
> > > >
> > > > I don't think it is a correct behavior for old drivers to use new DTBs or new
> > > > drivers to use old DTBs. Maybe you are correct, Frank also asked the same
> > > > question, let's see how Frank responded.
> > >
> > > DTBs should be considered stable ABI.
> > >
> >
> > ABI defined at binding doc.
> >   interrupt-names:
> >     oneOf:
> >       - items:
> >           - const: int0
> >       - items:
> >           - const: int0
> >           - const: pps
> >       - items:
> >           - const: int0
> >           - const: int1
> >           - const: int2
> >       - items:
> >           - const: int0
> >           - const: int1
> >           - const: int2
> >           - const: pps
> >
> > DTB should align binding doc. There are not 'descriptions' at 'interrupt',
> > which should match 'interrupt-names'. So IMX8MP dts have not match ABI,
> > which defined by binding doc. So it is DTS implement wrong.
>
> I follow your conclusion. But keep in mind, fixing the DTB would break
> compatibility. The wrong DTS looks like this:
>
> - const: int1
> - const: int2
> - const: int0
> - const: pps
>
> Currently we have broken DTS on the i.MX8M* and the
> FEC_QUIRK_WAKEUP_FROM_INT2 that "fixes" this.
>
> This patch uses this quirk to correct the IRQ <-> queue assignment in
> the driver.

This current code

for (i = 0; i < irq_cnt; i++) {
                snprintf(irq_name, sizeof(irq_name), "int%d", i);
                irq = platform_get_irq_byname_optional(pdev, irq_name);
		      ^^^^^^^^^^^^^^^^^^^^^

You just need add interrupt-names at imx8mp dts and reorder it to pass
DTB check.

                if (irq < 0)
                        irq = platform_get_irq(pdev, i);
                if (irq < 0) {
                        ret = irq;
                        goto failed_irq;
                }
                ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
                                       0, pdev->name, ndev);
                if (ret)
                        goto failed_irq;

                fep->irq[i] = irq;
        }

All irq handle by the same fec_enet_interrupt().  Change dts irq orders
doesn't broken compatiblity.

"pre-equeue" irq is new features. You can enable this feature only
when "interrupt-names" exist in future.

>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4020 bytes --]

On 17.10.2024 11:30:45, Frank Li wrote:
> On Thu, Oct 17, 2024 at 04:21:33PM +0200, Marc Kleine-Budde wrote:
> > On 17.10.2024 10:03:51, Frank Li wrote:
> > > > > > Yes, that is IMHO the correct description of the IP core, but the
> > > > > > i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> > > > > > reasons (fixed DTS with old driver) it's IMHO not possible to change the
> > > > > > DTS.
> > > > > >
> > > > >
> > > > > I don't think it is a correct behavior for old drivers to use new DTBs or new
> > > > > drivers to use old DTBs. Maybe you are correct, Frank also asked the same
> > > > > question, let's see how Frank responded.
> > > >
> > > > DTBs should be considered stable ABI.
> > > >
> > >
> > > ABI defined at binding doc.
> > >   interrupt-names:
> > >     oneOf:
> > >       - items:
> > >           - const: int0
> > >       - items:
> > >           - const: int0
> > >           - const: pps
> > >       - items:
> > >           - const: int0
> > >           - const: int1
> > >           - const: int2
> > >       - items:
> > >           - const: int0
> > >           - const: int1
> > >           - const: int2
> > >           - const: pps
> > >
> > > DTB should align binding doc. There are not 'descriptions' at 'interrupt',
> > > which should match 'interrupt-names'. So IMX8MP dts have not match ABI,
> > > which defined by binding doc. So it is DTS implement wrong.
> >
> > I follow your conclusion. But keep in mind, fixing the DTB would break
> > compatibility. The wrong DTS looks like this:
> >
> > - const: int1
> > - const: int2
> > - const: int0
> > - const: pps
> >
> > Currently we have broken DTS on the i.MX8M* and the
> > FEC_QUIRK_WAKEUP_FROM_INT2 that "fixes" this.
> >
> > This patch uses this quirk to correct the IRQ <-> queue assignment in
> > the driver.
> 
> This current code
> 
> for (i = 0; i < irq_cnt; i++) {
>                 snprintf(irq_name, sizeof(irq_name), "int%d", i);
>                 irq = platform_get_irq_byname_optional(pdev, irq_name);
> 		      ^^^^^^^^^^^^^^^^^^^^^
> 
> You just need add interrupt-names at imx8mp dts and reorder it to pass
> DTB check.

ACK

> 
>                 if (irq < 0)
>                         irq = platform_get_irq(pdev, i);
>                 if (irq < 0) {
>                         ret = irq;
>                         goto failed_irq;
>                 }
>                 ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
>                                        0, pdev->name, ndev);
>                 if (ret)
>                         goto failed_irq;
> 
>                 fep->irq[i] = irq;
>         }
> 
> All irq handle by the same fec_enet_interrupt().  Change dts irq orders
> doesn't broken compatiblity.

I'm sorry, but this is not 100% correct. Changing the _order_ of IRQs
does break compatibility. New DT (with changed IRQ order) with old
driver breaks wakeup functionality.

Have a look at b7cdc9658ac8 ("net: fec: add WoL support for i.MX8MQ"),
but keep in mind the patch description is not 100% correct:

| By default FEC driver treat irq[0] (i.e. int0 described in dt-binding)
| as wakeup interrupt, but this situation changed on i.MX8M serials, SoC
| integration guys mix wakeup interrupt signal into int2 interrupt line.
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This statement is wrong. The SoC integration is correct, the DT is
wrong.

| This patch introduces FEC_QUIRK_WAKEUP_FROM_INT2 to indicate int2 as
| wakeup interrupt for i.MX8MQ.

> "pre-equeue" irq is new features. You can enable this feature only
> when "interrupt-names" exist in future.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Frank Li]

On Fri, Oct 18, 2024 at 11:16:46AM +0200, Marc Kleine-Budde wrote:
> On 17.10.2024 11:30:45, Frank Li wrote:
> > On Thu, Oct 17, 2024 at 04:21:33PM +0200, Marc Kleine-Budde wrote:
> > > On 17.10.2024 10:03:51, Frank Li wrote:
> > > > > > > Yes, that is IMHO the correct description of the IP core, but the
> > > > > > > i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> > > > > > > reasons (fixed DTS with old driver) it's IMHO not possible to change the
> > > > > > > DTS.
> > > > > > >
> > > > > >
> > > > > > I don't think it is a correct behavior for old drivers to use new DTBs or new
> > > > > > drivers to use old DTBs. Maybe you are correct, Frank also asked the same
> > > > > > question, let's see how Frank responded.
> > > > >
> > > > > DTBs should be considered stable ABI.
> > > > >
> > > >
> > > > ABI defined at binding doc.
> > > >   interrupt-names:
> > > >     oneOf:
> > > >       - items:
> > > >           - const: int0
> > > >       - items:
> > > >           - const: int0
> > > >           - const: pps
> > > >       - items:
> > > >           - const: int0
> > > >           - const: int1
> > > >           - const: int2
> > > >       - items:
> > > >           - const: int0
> > > >           - const: int1
> > > >           - const: int2
> > > >           - const: pps
> > > >
> > > > DTB should align binding doc. There are not 'descriptions' at 'interrupt',
> > > > which should match 'interrupt-names'. So IMX8MP dts have not match ABI,
> > > > which defined by binding doc. So it is DTS implement wrong.
> > >
> > > I follow your conclusion. But keep in mind, fixing the DTB would break
> > > compatibility. The wrong DTS looks like this:
> > >
> > > - const: int1
> > > - const: int2
> > > - const: int0
> > > - const: pps
> > >
> > > Currently we have broken DTS on the i.MX8M* and the
> > > FEC_QUIRK_WAKEUP_FROM_INT2 that "fixes" this.
> > >
> > > This patch uses this quirk to correct the IRQ <-> queue assignment in
> > > the driver.
> >
> > This current code
> >
> > for (i = 0; i < irq_cnt; i++) {
> >                 snprintf(irq_name, sizeof(irq_name), "int%d", i);
> >                 irq = platform_get_irq_byname_optional(pdev, irq_name);
> > 		      ^^^^^^^^^^^^^^^^^^^^^
> >
> > You just need add interrupt-names at imx8mp dts and reorder it to pass
> > DTB check.
>
> ACK
>
> >
> >                 if (irq < 0)
> >                         irq = platform_get_irq(pdev, i);
> >                 if (irq < 0) {
> >                         ret = irq;
> >                         goto failed_irq;
> >                 }
> >                 ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
> >                                        0, pdev->name, ndev);
> >                 if (ret)
> >                         goto failed_irq;
> >
> >                 fep->irq[i] = irq;
> >         }
> >
> > All irq handle by the same fec_enet_interrupt().  Change dts irq orders
> > doesn't broken compatiblity.
>
> I'm sorry, but this is not 100% correct. Changing the _order_ of IRQs
> does break compatibility. New DT (with changed IRQ order) with old
> driver breaks wakeup functionality.
>
> Have a look at b7cdc9658ac8 ("net: fec: add WoL support for i.MX8MQ"),
> but keep in mind the patch description is not 100% correct:
>
> | By default FEC driver treat irq[0] (i.e. int0 described in dt-binding)
> | as wakeup interrupt, but this situation changed on i.MX8M serials, SoC
> | integration guys mix wakeup interrupt signal into int2 interrupt line.
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This statement is wrong. The SoC integration is correct, the DT is
> wrong.

We should fix wrong thing instead of continuing on the wrong path. No
much user use wakeup funcationlity. Actually you enable both int0 and int2
as wakeup source if try to keep compatility.

for example:
	fec->wake_irq = fep->irq[0];
	if (FEC_QUIRK_WAKEUP_FROM_INT2)
		fec->wake_irq2 = fep->irq[2];


...
	if (fep->wake_irq2 > 0) {
                                disable_irq(fep->wake_irq2);
                                enable_irq_wake(fep->wake_irq2);
                        }

I perfer fix dts and remove FEC_QUIRK_WAKEUP_FROM_INT2.


>
> | This patch introduces FEC_QUIRK_WAKEUP_FROM_INT2 to indicate int2 as
> | wakeup interrupt for i.MX8MQ.
>
> > "pre-equeue" irq is new features. You can enable this feature only
> > when "interrupt-names" exist in future.
>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[PATCH net-next 08/13] net: fec: fec_probe(): let IRQ name reflect its function

[Marc Kleine-Budde]

The FEC IP core in the i.MX7 and newer SoCs has 4 IRQs. The first 3
correspond to the 3 RX/TX queues, the 4th is for processing Pulses Per
Second. In addition, the 1st IRQ handles the remaining IRQ sources of
the IP core. They are all displayed with the same name in
/proc/interrupts:

| 208:          0          0          0          0     GICv3 150 Level     30be0000.ethernet
| 209:          0          0          0          0     GICv3 151 Level     30be0000.ethernet
| 210:       3898          0          0          0     GICv3 152 Level     30be0000.ethernet
| 211:          0          0          0          0     GICv3 153 Level     30be0000.ethernet

For easier debugging make the name of the IRQ reflect its function.
Use the postfix "-RxTx" and the queue number for the first 3 IRQs, add
"+misc" for the 1st IRQ. The postfix "-PPS" specifies the PPS IRQ.

With this change /proc/interrupts looks like this:

| 208:          2          0          0          0     GICv3 150 Level     30be0000.ethernet-RxTx1
| 209:          0          0          0          0     GICv3 151 Level     30be0000.ethernet-RxTx2
| 210:       3526          0          0          0     GICv3 152 Level     30be0000.ethernet-RxTx0+misc
| 211:          0          0          0          0     GICv3 153 Level     30be0000.ethernet-PPS

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 9 ++++++++-
 drivers/net/ethernet/freescale/fec_ptp.c  | 5 ++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f124ffe3619d82dc089f8494d33d2398e6f631fb..c8b2170735e599cd10492169ab32d0e20b28311b 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -4492,8 +4492,15 @@ fec_probe(struct platform_device *pdev)
 		goto failed_init;
 
 	for (i = 0; i < irq_cnt; i++) {
+		const char *dev_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-RxTx%d%s",
+						      pdev->name, i, i == 0 ? "+misc" : "");
 		int irq_num;
 
+		if (!dev_name) {
+			ret = -ENOMEM;
+			goto failed_irq;
+		}
+
 		if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ)
 			irq_num = (i + irq_cnt - 1) % irq_cnt;
 		else
@@ -4508,7 +4515,7 @@ fec_probe(struct platform_device *pdev)
 			goto failed_irq;
 		}
 		ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
-				       0, pdev->name, ndev);
+				       0, dev_name, ndev);
 		if (ret)
 			goto failed_irq;
 
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 8722f623d9e47e385439f1cee8c677e2b95b236d..0ac89fed366a83bcbfc900ea4409f4e98c4e14da 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -749,8 +749,11 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 	 * only the PTP_CLOCK_PPS clock events should stop
 	 */
 	if (irq >= 0) {
+		const char *dev_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+						      "%s-PPS", pdev->name);
+
 		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
-				       0, pdev->name, ndev);
+				       0, dev_name ? dev_name : pdev->name, ndev);
 		if (ret < 0)
 			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
 				 ret);

-- 
2.45.2

Re: [PATCH net-next 08/13] net: fec: fec_probe(): let IRQ name reflect its function

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:56PM +0200, Marc Kleine-Budde wrote:
> The FEC IP core in the i.MX7 and newer SoCs has 4 IRQs. The first 3
> correspond to the 3 RX/TX queues, the 4th is for processing Pulses Per
> Second. In addition, the 1st IRQ handles the remaining IRQ sources of
> the IP core. They are all displayed with the same name in
> /proc/interrupts:
>
> | 208:          0          0          0          0     GICv3 150 Level     30be0000.ethernet
> | 209:          0          0          0          0     GICv3 151 Level     30be0000.ethernet
> | 210:       3898          0          0          0     GICv3 152 Level     30be0000.ethernet
> | 211:          0          0          0          0     GICv3 153 Level     30be0000.ethernet
>
> For easier debugging make the name of the IRQ reflect its function.
> Use the postfix "-RxTx" and the queue number for the first 3 IRQs, add
> "+misc" for the 1st IRQ. The postfix "-PPS" specifies the PPS IRQ.
>
> With this change /proc/interrupts looks like this:
>
> | 208:          2          0          0          0     GICv3 150 Level     30be0000.ethernet-RxTx1
> | 209:          0          0          0          0     GICv3 151 Level     30be0000.ethernet-RxTx2
> | 210:       3526          0          0          0     GICv3 152 Level     30be0000.ethernet-RxTx0+misc
> | 211:          0          0          0          0     GICv3 153 Level     30be0000.ethernet-PPS

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 9 ++++++++-
>  drivers/net/ethernet/freescale/fec_ptp.c  | 5 ++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index f124ffe3619d82dc089f8494d33d2398e6f631fb..c8b2170735e599cd10492169ab32d0e20b28311b 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -4492,8 +4492,15 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_init;
>
>  	for (i = 0; i < irq_cnt; i++) {
> +		const char *dev_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-RxTx%d%s",
> +						      pdev->name, i, i == 0 ? "+misc" : "");
>  		int irq_num;
>
> +		if (!dev_name) {
> +			ret = -ENOMEM;
> +			goto failed_irq;
> +		}
> +
>  		if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ)
>  			irq_num = (i + irq_cnt - 1) % irq_cnt;
>  		else
> @@ -4508,7 +4515,7 @@ fec_probe(struct platform_device *pdev)
>  			goto failed_irq;
>  		}
>  		ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
> -				       0, pdev->name, ndev);
> +				       0, dev_name, ndev);
>  		if (ret)
>  			goto failed_irq;
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 8722f623d9e47e385439f1cee8c677e2b95b236d..0ac89fed366a83bcbfc900ea4409f4e98c4e14da 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -749,8 +749,11 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>  	 * only the PTP_CLOCK_PPS clock events should stop
>  	 */
>  	if (irq >= 0) {
> +		const char *dev_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +						      "%s-PPS", pdev->name);
> +
>  		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
> -				       0, pdev->name, ndev);
> +				       0, dev_name ? dev_name : pdev->name, ndev);
>  		if (ret < 0)
>  			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
>  				 ret);
>
> --
> 2.45.2
>
>

RE: [PATCH net-next 08/13] net: fec: fec_probe(): let IRQ name reflect its function

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 08/13] net: fec: fec_probe(): let IRQ name reflect its
> function
> 
> The FEC IP core in the i.MX7 and newer SoCs has 4 IRQs. The first 3
> correspond to the 3 RX/TX queues, the 4th is for processing Pulses Per
> Second. In addition, the 1st IRQ handles the remaining IRQ sources of
> the IP core. They are all displayed with the same name in
> /proc/interrupts:
> 
> | 208:          0          0          0          0     GICv3 150
> Level     30be0000.ethernet
> | 209:          0          0          0          0     GICv3 151
> Level     30be0000.ethernet
> | 210:       3898          0          0          0     GICv3 152
> Level     30be0000.ethernet
> | 211:          0          0          0          0     GICv3 153
> Level     30be0000.ethernet
> 
> For easier debugging make the name of the IRQ reflect its function.
> Use the postfix "-RxTx" and the queue number for the first 3 IRQs, add
> "+misc" for the 1st IRQ. The postfix "-PPS" specifies the PPS IRQ.
> 
> With this change /proc/interrupts looks like this:
> 
> | 208:          2          0          0          0     GICv3 150
> Level     30be0000.ethernet-RxTx1
> | 209:          0          0          0          0     GICv3 151
> Level     30be0000.ethernet-RxTx2
> | 210:       3526          0          0          0     GICv3 152
> Level     30be0000.ethernet-RxTx0+misc
> | 211:          0          0          0          0     GICv3 153
> Level     30be0000.ethernet-PPS
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 9 ++++++++-
>  drivers/net/ethernet/freescale/fec_ptp.c  | 5 ++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> f124ffe3619d82dc089f8494d33d2398e6f631fb..c8b2170735e599cd1049216
> 9ab32d0e20b28311b 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -4492,8 +4492,15 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_init;
> 
>  	for (i = 0; i < irq_cnt; i++) {
> +		const char *dev_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> "%s-RxTx%d%s",
> +						      pdev->name, i, i == 0 ? "+misc" : "");
>  		int irq_num;
> 
> +		if (!dev_name) {
> +			ret = -ENOMEM;
> +			goto failed_irq;
> +		}
> +
>  		if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ)
>  			irq_num = (i + irq_cnt - 1) % irq_cnt;
>  		else
> @@ -4508,7 +4515,7 @@ fec_probe(struct platform_device *pdev)
>  			goto failed_irq;
>  		}
>  		ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
> -				       0, pdev->name, ndev);
> +				       0, dev_name, ndev);
>  		if (ret)
>  			goto failed_irq;
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index
> 8722f623d9e47e385439f1cee8c677e2b95b236d..0ac89fed366a83bcbfc900e
> a4409f4e98c4e14da 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -749,8 +749,11 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
>  	 * only the PTP_CLOCK_PPS clock events should stop
>  	 */
>  	if (irq >= 0) {
> +		const char *dev_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +						      "%s-PPS", pdev->name);
> +
>  		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
> -				       0, pdev->name, ndev);
> +				       0, dev_name ? dev_name : pdev->name, ndev);
>  		if (ret < 0)
>  			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
>  				 ret);
> 
> --
> 2.45.2
> 

Great, thanks!

Reviewed-by: Wei Fang <wei.fang@nxp.com>

[PATCH net-next 09/13] net: fec: fec_enet_rx_queue(): use same signature as fec_enet_tx_queue()

[Marc Kleine-Budde]

There are the functions fec_enet_rx_queue() and fec_enet_tx_queue(),
one for handling the RX queue the other one handles the TX queue.

However they don't have the same signature. To make the code more
readable make the signature of fec_enet_rx_queue() identical to the
signature of fec_enet_tx_queue().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c8b2170735e599cd10492169ab32d0e20b28311b..eb26e869c026225194f4df66db145494408bfe8a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1678,7 +1678,7 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
  * effectively tossing the packet.
  */
 static int
-fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
+fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct fec_enet_priv_rx_q *rxq;
@@ -1899,7 +1899,7 @@ static int fec_enet_rx(struct net_device *ndev, int budget)
 
 	/* Make sure that AVB queues are processed first. */
 	for (i = fep->num_rx_queues - 1; i >= 0; i--)
-		done += fec_enet_rx_queue(ndev, budget - done, i);
+		done += fec_enet_rx_queue(ndev, i, budget - done);
 
 	return done;
 }

-- 
2.45.2

Re: [PATCH net-next 09/13] net: fec: fec_enet_rx_queue(): use same signature as fec_enet_tx_queue()

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:57PM +0200, Marc Kleine-Budde wrote:
> There are the functions fec_enet_rx_queue() and fec_enet_tx_queue(),
> one for handling the RX queue the other one handles the TX queue.
>
> However they don't have the same signature. To make the code more
> readable make the signature of fec_enet_rx_queue() identical to the
> signature of fec_enet_tx_queue().

'signature' is strange here.

Align fec_enet_rx_queue() argument order with fec_enet_tx_queue() to make
code more readable.

Frank

>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index c8b2170735e599cd10492169ab32d0e20b28311b..eb26e869c026225194f4df66db145494408bfe8a 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1678,7 +1678,7 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
>   * effectively tossing the packet.
>   */
>  static int
> -fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
> +fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	struct fec_enet_priv_rx_q *rxq;
> @@ -1899,7 +1899,7 @@ static int fec_enet_rx(struct net_device *ndev, int budget)
>
>  	/* Make sure that AVB queues are processed first. */
>  	for (i = fep->num_rx_queues - 1; i >= 0; i--)
> -		done += fec_enet_rx_queue(ndev, budget - done, i);
> +		done += fec_enet_rx_queue(ndev, i, budget - done);
>
>  	return done;
>  }
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 09/13] net: fec: fec_enet_rx_queue(): use same signature as fec_enet_tx_queue()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On 16.10.2024 22:32:20, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:51:57PM +0200, Marc Kleine-Budde wrote:
> > There are the functions fec_enet_rx_queue() and fec_enet_tx_queue(),
> > one for handling the RX queue the other one handles the TX queue.
> >
> > However they don't have the same signature. To make the code more
> > readable make the signature of fec_enet_rx_queue() identical to the
> > signature of fec_enet_tx_queue().
> 
> 'signature' is strange here.

Signature, or the full name 'type signature', defines the types and
order of a function's arguments, see:

| https://en.wikipedia.org/wiki/Type_signature

> Align fec_enet_rx_queue() argument order with fec_enet_tx_queue() to make
> code more readable.

I've updated the last sentence.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 10/13] net: fec: fec_enet_rx_queue(): replace open coded cast by skb_vlan_eth_hdr()

[Marc Kleine-Budde]

In order to clean up the VLAN handling, replace an open coded cast
from skb->data to the vlan header with skb_vlan_eth_hdr().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index eb26e869c026225194f4df66db145494408bfe8a..fd7a78ec5fa8ac0f7d141779938a4690594dbef1 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1819,8 +1819,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 		    fep->bufdesc_ex &&
 		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
 			/* Push and remove the vlan tag */
-			struct vlan_hdr *vlan_header =
-					(struct vlan_hdr *) (data + ETH_HLEN);
+			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
 			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
 
 			vlan_packet_rcvd = true;

-- 
2.45.2

Re: [PATCH net-next 10/13] net: fec: fec_enet_rx_queue(): replace open coded cast by skb_vlan_eth_hdr()

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:58PM +0200, Marc Kleine-Budde wrote:
> In order to clean up the VLAN handling, replace an open coded cast
> from skb->data to the vlan header with skb_vlan_eth_hdr().

Replace manual VLAN header calculation with skb_vlan_eth_hdr()

Use the provided helper function skb_vlan_eth_hdr() to replace manual VLAN
header calculation for better readability and maintainability.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index eb26e869c026225194f4df66db145494408bfe8a..fd7a78ec5fa8ac0f7d141779938a4690594dbef1 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1819,8 +1819,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  		    fep->bufdesc_ex &&
>  		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
>  			/* Push and remove the vlan tag */
> -			struct vlan_hdr *vlan_header =
> -					(struct vlan_hdr *) (data + ETH_HLEN);
> +			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
>  			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
>
>  			vlan_packet_rcvd = true;
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 10/13] net: fec: fec_enet_rx_queue(): replace open coded cast by skb_vlan_eth_hdr()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

On 16.10.2024 22:44:18, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:51:58PM +0200, Marc Kleine-Budde wrote:
> > In order to clean up the VLAN handling, replace an open coded cast
> > from skb->data to the vlan header with skb_vlan_eth_hdr().
> 
> Replace manual VLAN header calculation with skb_vlan_eth_hdr()
> 
> Use the provided helper function skb_vlan_eth_hdr() to replace manual VLAN
> header calculation for better readability and maintainability.

Fixed,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH net-next 10/13] net: fec: fec_enet_rx_queue(): replace open coded cast by skb_vlan_eth_hdr()

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 10/13] net: fec: fec_enet_rx_queue(): replace open
> coded cast by skb_vlan_eth_hdr()
> 
> In order to clean up the VLAN handling, replace an open coded cast
> from skb->data to the vlan header with skb_vlan_eth_hdr().
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> eb26e869c026225194f4df66db145494408bfe8a..fd7a78ec5fa8ac0f7d141779
> 938a4690594dbef1 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1819,8 +1819,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
>  		    fep->bufdesc_ex &&
>  		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
>  			/* Push and remove the vlan tag */
> -			struct vlan_hdr *vlan_header =
> -					(struct vlan_hdr *) (data + ETH_HLEN);
> +			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
>  			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> 
>  			vlan_packet_rcvd = true;
> 
> --
> 2.45.2
> 

Reviewed-by: Wei Fang <wei.fang@nxp.com>

[PATCH net-next 11/13] net: fec: fec_enet_rx_queue(): reduce scope of data

[Marc Kleine-Budde]

In order to clean up of the VLAN handling, reduce the scope of data.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index fd7a78ec5fa8ac0f7d141779938a4690594dbef1..640fbde10861005e7e2eb23358bfeaac49ec1792 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1686,7 +1686,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 	unsigned short status;
 	struct  sk_buff *skb;
 	ushort	pkt_len;
-	__u8 *data;
 	int	pkt_received = 0;
 	struct	bufdesc_ex *ebdp = NULL;
 	bool	vlan_packet_rcvd = false;
@@ -1803,10 +1802,11 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 		skb_mark_for_recycle(skb);
 
 		if (unlikely(need_swap)) {
+			u8 *data;
+
 			data = page_address(page) + FEC_ENET_XDP_HEADROOM;
 			swap_buffer(data, pkt_len);
 		}
-		data = skb->data;
 
 		/* Extract the enhanced buffer descriptor */
 		ebdp = NULL;
@@ -1824,7 +1824,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 
 			vlan_packet_rcvd = true;
 
-			memmove(skb->data + VLAN_HLEN, data, ETH_ALEN * 2);
+			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
 			skb_pull(skb, VLAN_HLEN);
 		}
 

-- 
2.45.2

Re: [PATCH net-next 11/13] net: fec: fec_enet_rx_queue(): reduce scope of data

[Frank Li]

On Wed, Oct 16, 2024 at 11:51:59PM +0200, Marc Kleine-Budde wrote:
> In order to clean up of the VLAN handling, reduce the scope of data.
>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index fd7a78ec5fa8ac0f7d141779938a4690594dbef1..640fbde10861005e7e2eb23358bfeaac49ec1792 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1686,7 +1686,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  	unsigned short status;
>  	struct  sk_buff *skb;
>  	ushort	pkt_len;
> -	__u8 *data;
>  	int	pkt_received = 0;
>  	struct	bufdesc_ex *ebdp = NULL;
>  	bool	vlan_packet_rcvd = false;
> @@ -1803,10 +1802,11 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  		skb_mark_for_recycle(skb);
>
>  		if (unlikely(need_swap)) {
> +			u8 *data;
> +
>  			data = page_address(page) + FEC_ENET_XDP_HEADROOM;
>  			swap_buffer(data, pkt_len);
>  		}
> -		data = skb->data;
>
>  		/* Extract the enhanced buffer descriptor */
>  		ebdp = NULL;
> @@ -1824,7 +1824,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>
>  			vlan_packet_rcvd = true;
>
> -			memmove(skb->data + VLAN_HLEN, data, ETH_ALEN * 2);
> +			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
>  			skb_pull(skb, VLAN_HLEN);
>  		}
>
>
> --
> 2.45.2
>
>

RE: [PATCH net-next 11/13] net: fec: fec_enet_rx_queue(): reduce scope of data

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 11/13] net: fec: fec_enet_rx_queue(): reduce scope of
> data
> 
> In order to clean up of the VLAN handling, reduce the scope of data.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> fd7a78ec5fa8ac0f7d141779938a4690594dbef1..640fbde10861005e7e2eb23
> 358bfeaac49ec1792 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1686,7 +1686,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
>  	unsigned short status;
>  	struct  sk_buff *skb;
>  	ushort	pkt_len;
> -	__u8 *data;
>  	int	pkt_received = 0;
>  	struct	bufdesc_ex *ebdp = NULL;
>  	bool	vlan_packet_rcvd = false;
> @@ -1803,10 +1802,11 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
>  		skb_mark_for_recycle(skb);
> 
>  		if (unlikely(need_swap)) {
> +			u8 *data;
> +
>  			data = page_address(page) + FEC_ENET_XDP_HEADROOM;
>  			swap_buffer(data, pkt_len);
>  		}
> -		data = skb->data;
> 
>  		/* Extract the enhanced buffer descriptor */
>  		ebdp = NULL;
> @@ -1824,7 +1824,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
> 
>  			vlan_packet_rcvd = true;
> 
> -			memmove(skb->data + VLAN_HLEN, data, ETH_ALEN * 2);
> +			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
>  			skb_pull(skb, VLAN_HLEN);
>  		}
> 
> 
> --
> 2.45.2
> 
Thanks

Reviewed-by: Wei Fang <wei.fang@nxp.com>

[PATCH net-next 12/13] net: fec: fec_enet_rx_queue(): move_call to _vlan_hwaccel_put_tag()

[Marc Kleine-Budde]

To clean up the VLAN handling, move the call to
__vlan_hwaccel_put_tag() into the body of the if statement, which
checks for VLAN handling in the first place.

This allows to remove vlan_packet_rcvd and reduce the scope of
vlan_tag.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 640fbde10861005e7e2eb23358bfeaac49ec1792..d9415c7c16cea3fc3d91e198c21af9fe9e21747e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1688,8 +1688,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 	ushort	pkt_len;
 	int	pkt_received = 0;
 	struct	bufdesc_ex *ebdp = NULL;
-	bool	vlan_packet_rcvd = false;
-	u16	vlan_tag;
 	int	index = 0;
 	bool	need_swap = fep->quirks & FEC_QUIRK_SWAP_FRAME;
 	struct bpf_prog *xdp_prog = READ_ONCE(fep->xdp_prog);
@@ -1814,18 +1812,18 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 			ebdp = (struct bufdesc_ex *)bdp;
 
 		/* If this is a VLAN packet remove the VLAN Tag */
-		vlan_packet_rcvd = false;
 		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
 		    fep->bufdesc_ex &&
 		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
 			/* Push and remove the vlan tag */
 			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
-			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
-
-			vlan_packet_rcvd = true;
+			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
 
 			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
 			skb_pull(skb, VLAN_HLEN);
+			__vlan_hwaccel_put_tag(skb,
+					       htons(ETH_P_8021Q),
+					       vlan_tag);
 		}
 
 		skb->protocol = eth_type_trans(skb, ndev);
@@ -1845,12 +1843,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 			}
 		}
 
-		/* Handle received VLAN packets */
-		if (vlan_packet_rcvd)
-			__vlan_hwaccel_put_tag(skb,
-					       htons(ETH_P_8021Q),
-					       vlan_tag);
-
 		skb_record_rx_queue(skb, queue_id);
 		napi_gro_receive(&fep->napi, skb);
 

-- 
2.45.2

Re: [PATCH net-next 12/13] net: fec: fec_enet_rx_queue(): move_call to _vlan_hwaccel_put_tag()

[Frank Li]

On Wed, Oct 16, 2024 at 11:52:00PM +0200, Marc Kleine-Budde wrote:
> To clean up the VLAN handling, move the call to
> __vlan_hwaccel_put_tag() into the body of the if statement, which
> checks for VLAN handling in the first place.
>
> This allows to remove vlan_packet_rcvd and reduce the scope of
> vlan_tag.

Move __vlan_hwaccel_put_tag() into the if statement that sets
vlan_packet_rcvd=true. This change eliminates the unnecessary
vlan_packet_rcvd variable, simplifying the code and improving clarity.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 640fbde10861005e7e2eb23358bfeaac49ec1792..d9415c7c16cea3fc3d91e198c21af9fe9e21747e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1688,8 +1688,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  	ushort	pkt_len;
>  	int	pkt_received = 0;
>  	struct	bufdesc_ex *ebdp = NULL;
> -	bool	vlan_packet_rcvd = false;
> -	u16	vlan_tag;
>  	int	index = 0;
>  	bool	need_swap = fep->quirks & FEC_QUIRK_SWAP_FRAME;
>  	struct bpf_prog *xdp_prog = READ_ONCE(fep->xdp_prog);
> @@ -1814,18 +1812,18 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  			ebdp = (struct bufdesc_ex *)bdp;
>
>  		/* If this is a VLAN packet remove the VLAN Tag */
> -		vlan_packet_rcvd = false;
>  		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
>  		    fep->bufdesc_ex &&
>  		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
>  			/* Push and remove the vlan tag */
>  			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> -			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> -
> -			vlan_packet_rcvd = true;
> +			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
>
>  			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
>  			skb_pull(skb, VLAN_HLEN);
> +			__vlan_hwaccel_put_tag(skb,
> +					       htons(ETH_P_8021Q),
> +					       vlan_tag);
>  		}
>
>  		skb->protocol = eth_type_trans(skb, ndev);
> @@ -1845,12 +1843,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  			}
>  		}
>
> -		/* Handle received VLAN packets */
> -		if (vlan_packet_rcvd)
> -			__vlan_hwaccel_put_tag(skb,
> -					       htons(ETH_P_8021Q),
> -					       vlan_tag);
> -
>  		skb_record_rx_queue(skb, queue_id);
>  		napi_gro_receive(&fep->napi, skb);
>
>
> --
> 2.45.2
>
>

Re: [PATCH net-next 12/13] net: fec: fec_enet_rx_queue(): move_call to _vlan_hwaccel_put_tag()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On 16.10.2024 23:05:27, Frank Li wrote:
> On Wed, Oct 16, 2024 at 11:52:00PM +0200, Marc Kleine-Budde wrote:
> > To clean up the VLAN handling, move the call to
> > __vlan_hwaccel_put_tag() into the body of the if statement, which
> > checks for VLAN handling in the first place.
> >
> > This allows to remove vlan_packet_rcvd and reduce the scope of
> > vlan_tag.
> 
> Move __vlan_hwaccel_put_tag() into the if statement that sets
> vlan_packet_rcvd=true. This change eliminates the unnecessary
> vlan_packet_rcvd variable, simplifying the code and improving clarity.

Fixed,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH net-next 12/13] net: fec: fec_enet_rx_queue(): move_call to _vlan_hwaccel_put_tag()

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 12/13] net: fec: fec_enet_rx_queue(): move_call to
> _vlan_hwaccel_put_tag()
> 
> To clean up the VLAN handling, move the call to
> __vlan_hwaccel_put_tag() into the body of the if statement, which
> checks for VLAN handling in the first place.
> 
> This allows to remove vlan_packet_rcvd and reduce the scope of
> vlan_tag.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> 640fbde10861005e7e2eb23358bfeaac49ec1792..d9415c7c16cea3fc3d91e19
> 8c21af9fe9e21747e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1688,8 +1688,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
>  	ushort	pkt_len;
>  	int	pkt_received = 0;
>  	struct	bufdesc_ex *ebdp = NULL;
> -	bool	vlan_packet_rcvd = false;
> -	u16	vlan_tag;
>  	int	index = 0;
>  	bool	need_swap = fep->quirks & FEC_QUIRK_SWAP_FRAME;
>  	struct bpf_prog *xdp_prog = READ_ONCE(fep->xdp_prog);
> @@ -1814,18 +1812,18 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
>  			ebdp = (struct bufdesc_ex *)bdp;
> 
>  		/* If this is a VLAN packet remove the VLAN Tag */
> -		vlan_packet_rcvd = false;
>  		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
>  		    fep->bufdesc_ex &&
>  		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
>  			/* Push and remove the vlan tag */
>  			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> -			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> -
> -			vlan_packet_rcvd = true;
> +			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> 
>  			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
>  			skb_pull(skb, VLAN_HLEN);
> +			__vlan_hwaccel_put_tag(skb,
> +					       htons(ETH_P_8021Q),
> +					       vlan_tag);
>  		}
> 
>  		skb->protocol = eth_type_trans(skb, ndev);
> @@ -1845,12 +1843,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
>  			}
>  		}
> 
> -		/* Handle received VLAN packets */
> -		if (vlan_packet_rcvd)
> -			__vlan_hwaccel_put_tag(skb,
> -					       htons(ETH_P_8021Q),
> -					       vlan_tag);
> -
>  		skb_record_rx_queue(skb, queue_id);
>  		napi_gro_receive(&fep->napi, skb);
> 
> 
> --
> 2.45.2
> 

Thanks

Reviewed-by: Wei Fang <wei.fang@nxp.com>

[PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out VLAN handling into separate function fec_enet_rx_vlan()

[Marc Kleine-Budde]

In order to clean up of the VLAN handling, factor out the VLAN
handling into separate function fec_enet_rx_vlan().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 32 ++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e62924c3b4597bb580 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
 	return ret;
 }
 
+static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
+{
+	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
+
+	if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
+		/* Push and remove the vlan tag */
+		u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
+
+		memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
+		skb_pull(skb, VLAN_HLEN);
+		__vlan_hwaccel_put_tag(skb,
+				       htons(ETH_P_8021Q),
+				       vlan_tag);
+	}
+}
+
 /* During a receive, the bd_rx.cur points to the current incoming buffer.
  * When we update through the ring, if the next incoming buffer has
  * not been given to the system, we just set the empty indicator,
@@ -1812,19 +1828,9 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 			ebdp = (struct bufdesc_ex *)bdp;
 
 		/* If this is a VLAN packet remove the VLAN Tag */
-		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
-		    fep->bufdesc_ex &&
-		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
-			/* Push and remove the vlan tag */
-			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
-			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
-
-			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
-			skb_pull(skb, VLAN_HLEN);
-			__vlan_hwaccel_put_tag(skb,
-					       htons(ETH_P_8021Q),
-					       vlan_tag);
-		}
+		if (fep->bufdesc_ex &&
+		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN)))
+			fec_enet_rx_vlan(ndev, skb);
 
 		skb->protocol = eth_type_trans(skb, ndev);
 

-- 
2.45.2

Re: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out VLAN handling into separate function fec_enet_rx_vlan()

[Frank Li]

On Wed, Oct 16, 2024 at 11:52:01PM +0200, Marc Kleine-Budde wrote:
> In order to clean up of the VLAN handling, factor out the VLAN
> handling into separate function fec_enet_rx_vlan().
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/net/ethernet/freescale/fec_main.c | 32 ++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e62924c3b4597bb580 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
>  	return ret;
>  }
>
> +static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
> +{
> +	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> +
> +	if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
> +		/* Push and remove the vlan tag */
> +		u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> +
> +		memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
> +		skb_pull(skb, VLAN_HLEN);
> +		__vlan_hwaccel_put_tag(skb,
> +				       htons(ETH_P_8021Q),
> +				       vlan_tag);
> +	}
> +}
> +
>  /* During a receive, the bd_rx.cur points to the current incoming buffer.
>   * When we update through the ring, if the next incoming buffer has
>   * not been given to the system, we just set the empty indicator,
> @@ -1812,19 +1828,9 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  			ebdp = (struct bufdesc_ex *)bdp;
>
>  		/* If this is a VLAN packet remove the VLAN Tag */
> -		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
> -		    fep->bufdesc_ex &&
> -		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
> -			/* Push and remove the vlan tag */
> -			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> -			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> -
> -			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
> -			skb_pull(skb, VLAN_HLEN);
> -			__vlan_hwaccel_put_tag(skb,
> -					       htons(ETH_P_8021Q),
> -					       vlan_tag);
> -		}
> +		if (fep->bufdesc_ex &&
> +		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN)))
> +			fec_enet_rx_vlan(ndev, skb);
>
>  		skb->protocol = eth_type_trans(skb, ndev);
>
>
> --
> 2.45.2
>
>

RE: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out VLAN handling into separate function fec_enet_rx_vlan()

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out
> VLAN handling into separate function fec_enet_rx_vlan()
> 
> In order to clean up of the VLAN handling, factor out the VLAN
> handling into separate function fec_enet_rx_vlan().
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 32
> ++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e
> 62924c3b4597bb580 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private *fep,
> struct bpf_prog *prog,
>  	return ret;
>  }
> 
> +static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
> +{
> +	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);

Why not move vlan_header into the if statement?

> +
> +	if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
> +		/* Push and remove the vlan tag */
> +		u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> +
> +		memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
> +		skb_pull(skb, VLAN_HLEN);
> +		__vlan_hwaccel_put_tag(skb,
> +				       htons(ETH_P_8021Q),
> +				       vlan_tag);
> +	}
> +}
> +
>  /* During a receive, the bd_rx.cur points to the current incoming buffer.
>   * When we update through the ring, if the next incoming buffer has
>   * not been given to the system, we just set the empty indicator,
> @@ -1812,19 +1828,9 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
>  			ebdp = (struct bufdesc_ex *)bdp;
> 
>  		/* If this is a VLAN packet remove the VLAN Tag */
> -		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
> -		    fep->bufdesc_ex &&
> -		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
> -			/* Push and remove the vlan tag */
> -			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> -			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> -
> -			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
> -			skb_pull(skb, VLAN_HLEN);
> -			__vlan_hwaccel_put_tag(skb,
> -					       htons(ETH_P_8021Q),
> -					       vlan_tag);
> -		}
> +		if (fep->bufdesc_ex &&
> +		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN)))
> +			fec_enet_rx_vlan(ndev, skb);
> 
>  		skb->protocol = eth_type_trans(skb, ndev);
> 
> 
> --
> 2.45.2
> 

Re: RE: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out VLAN handling into separate function fec_enet_rx_vlan()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]

On 17.10.2024 03:33:09, Wei Fang wrote:
> > -----Original Message-----
> > From: Marc Kleine-Budde <mkl@pengutronix.de>
> > Sent: 2024年10月17日 5:52
> > To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> > Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> > Cochran <richardcochran@gmail.com>
> > Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> > Subject: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out
> > VLAN handling into separate function fec_enet_rx_vlan()
> > 
> > In order to clean up of the VLAN handling, factor out the VLAN
> > handling into separate function fec_enet_rx_vlan().
> > 
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 32
> > ++++++++++++++++++-------------
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index
> > d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e
> > 62924c3b4597bb580 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private *fep,
> > struct bpf_prog *prog,
> >  	return ret;
> >  }
> > 
> > +static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
> > +{
> > +	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> 
> Why not move vlan_header into the if statement?

I've an upcoming patch that adds NETIF_F_HW_VLAN_STAG_RX (a.k.a.
801.2ad, S-VLAN) handling that changes this function.

One hunk looks like this, it uses the vlan_header outside of the if:

@@ -1675,15 +1678,19 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
 static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
 {
         struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
+        __be16 vlan_proto = vlan_header->h_vlan_proto;
 
-        if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
+        if ((vlan_proto == htons(ETH_P_8021Q) &&
+             ndev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
+            (vlan_proto == htons(ETH_P_8021AD) &&
+             ndev->features & NETIF_F_HW_VLAN_STAG_RX)) {
                 /* Push and remove the vlan tag */
                 u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: RE: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out VLAN handling into separate function fec_enet_rx_vlan()

[Wei Fang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 15:09
> To: Wei Fang <wei.fang@nxp.com>
> Cc: Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Richard Cochran <richardcochran@gmail.com>;
> imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: RE: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor
> out VLAN handling into separate function fec_enet_rx_vlan()
> 
> On 17.10.2024 03:33:09, Wei Fang wrote:
> > > -----Original Message-----
> > > From: Marc Kleine-Budde <mkl@pengutronix.de>
> > > Sent: 2024年10月17日 5:52
> > > To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang
> <shenwei.wang@nxp.com>;
> > > Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> > > Cochran <richardcochran@gmail.com>
> > > Cc: imx@lists.linux.dev; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org;
> > > kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> > > Subject: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out
> > > VLAN handling into separate function fec_enet_rx_vlan()
> > >
> > > In order to clean up of the VLAN handling, factor out the VLAN
> > > handling into separate function fec_enet_rx_vlan().
> > >
> > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > ---
> > >  drivers/net/ethernet/freescale/fec_main.c | 32
> > > ++++++++++++++++++-------------
> > >  1 file changed, 19 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > > b/drivers/net/ethernet/freescale/fec_main.c
> > > index
> > >
> d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e
> > > 62924c3b4597bb580 100644
> > > --- a/drivers/net/ethernet/freescale/fec_main.c
> > > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > > @@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private
> *fep,
> > > struct bpf_prog *prog,
> > >  	return ret;
> > >  }
> > >
> > > +static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
> > > +{
> > > +	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> >
> > Why not move vlan_header into the if statement?
> 
> I've an upcoming patch that adds NETIF_F_HW_VLAN_STAG_RX (a.k.a.
> 801.2ad, S-VLAN) handling that changes this function.
> 
> One hunk looks like this, it uses the vlan_header outside of the if:
> 

You can move valn_header out of the 'if' when it happens.

> @@ -1675,15 +1678,19 @@ fec_enet_run_xdp(struct fec_enet_private *fep,
> struct bpf_prog *prog,
>  static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
>  {
>          struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> +        __be16 vlan_proto = vlan_header->h_vlan_proto;
> 
> -        if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
> +        if ((vlan_proto == htons(ETH_P_8021Q) &&
> +             ndev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
> +            (vlan_proto == htons(ETH_P_8021AD) &&
> +             ndev->features & NETIF_F_HW_VLAN_STAG_RX)) {
>                  /* Push and remove the vlan tag */
>                  u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> 
> regards,
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: RE: RE: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out VLAN handling into separate function fec_enet_rx_vlan()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 675 bytes --]

On 17.10.2024 07:48:34, Wei Fang wrote:
> > > Why not move vlan_header into the if statement?
> > 
> > I've an upcoming patch that adds NETIF_F_HW_VLAN_STAG_RX (a.k.a.
> > 801.2ad, S-VLAN) handling that changes this function.
> > 
> > One hunk looks like this, it uses the vlan_header outside of the if:
> > 
> 
> You can move valn_header out of the 'if' when it happens.

Ok.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]