| Message ID | 9848F2DB572E5649BA045B288BE08FBE014FE4E1@039-SN2MPN1-023.039d.mgd.msft.net |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Jun 26, 2013 at 02:31:40AM +0000, Duan Fugang-B38611 wrote: > In addition, > 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. I guess Jim only tested the code on imx6q-fec. We shouldn't enable it for the other two platforms until it's been tested on them. As the counter example, although we expect FEC_QUIRK_HAS_CSUM should also work for imx28-fec, if we enable FEC_QUIRK_HAS_CSUM for it, fec driver will just break right way on imx28. Shawn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yes, you are right. Before we sent our our patch, we must test it on all covered platforms. Thanks, Andy -----Original Message----- From: Shawn Guo [mailto:shawn.guo@linaro.org] Sent: Wednesday, June 26, 2013 10:57 AM To: Duan Fugang-B38611 Cc: Jim Baxter; David S. Miller; Li Frank-B20596; Estevam Fabio-R49496; Lucas Stach; netdev@vger.kernel.org; Ben Hutchings Subject: Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support. On Wed, Jun 26, 2013 at 02:31:40AM +0000, Duan Fugang-B38611 wrote: > In addition, > 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. I guess Jim only tested the code on imx6q-fec. We shouldn't enable it for the other two platforms until it's been tested on them. As the counter example, although we expect FEC_QUIRK_HAS_CSUM should also work for imx28-fec, if we enable FEC_QUIRK_HAS_CSUM for it, fec driver will just break right way on imx28. Shawn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Do not top post, thank you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/06/13 03:31, Duan Fugang-B38611 wrote: > In addition, > 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. I have only tested it on the i.MX6, if someone wants to check it on the imx28 later they can add it to that option. > 2. Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user. What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by > */ > -#define PKT_MAXBUF_SIZE 1518 > +#define PKT_MAXBUF_SIZE 1522 > #define PKT_MINBUF_SIZE 64 > -#define PKT_MAXBLR_SIZE 1520 > +#define PKT_MAXBLR_SIZE 1540 > [Andy]Pls define the MAXBLR size to 1536, which is multiplied by 64 > #define PKT_MAXBLR_SIZE 1536 I will change this. > + int payload_offset = (2 * ETH_ALEN); > skb_reserve(skb, NET_IP_ALIGN); > skb_put(skb, pkt_len - 4); /* Make room */ > - skb_copy_to_linear_data(skb, data, pkt_len - 4); > + > + /* Extract the frame data without the VLAN header. */ > + skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN)); > + if (vlan_packet_rcvd) > + payload_offset = (2 * ETH_ALEN) + VLAN_HLEN; > + skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN), > + data + payload_offset, > + pkt_len - 4 - (2 * ETH_ALEN)); > + > [Andy] because the IP don't support HW VLAN tag extraction, it need sw to do it. It is ok for me. Yes it needs to be done in software as the Ethernet card only detects the VLAN packet, doing it here is more efficient then the kernel doing it. Thank you, Jim -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/26/13 18:09, Jim Baxter wrote: > > In addition, > > 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. > I have only tested it on the i.MX6, if someone wants to check it on the > imx28 later they can add it to that option. > > 2. Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user. > What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by You can enable the feature like below, which is reasonable. .ndo_fix_features = fec_enet_fix_features static netdev_features_t fec_enet_fix_features (struct net_device *dev, netdev_features_t features) { struct fec_enet_private *fep = netdev_priv(dev); const struct platform_device_id *id_entry = platform_get_device_id(fep->pdev); if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) features |= NETIF_F_HW_VLAN_CTAG_RX; return features; } Thank you, Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/06/13 12:18, Duan Fugang-B38611 wrote: > On 06/26/13 18:09, Jim Baxter wrote: > >>> In addition, >>> 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. >> I have only tested it on the i.MX6, if someone wants to check it on the >> imx28 later they can add it to that option. > >>> 2. Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user. >> What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by > > You can enable the feature like below, which is reasonable. > .ndo_fix_features = fec_enet_fix_features > > static netdev_features_t fec_enet_fix_features (struct net_device *dev, > netdev_features_t features) > { > struct fec_enet_private *fep = netdev_priv(dev); > const struct platform_device_id *id_entry = > platform_get_device_id(fep->pdev); > > if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) > features |= NETIF_F_HW_VLAN_CTAG_RX; > > return features; > } > > > Thank you, > Andy > > I currently set the features in fec_enet_init() along with setting hw_features, is fec_enet_fix_features() a more correct design for driver implementation point? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/26/13 19:45, Jim Baxter wrote: > On 26/06/13 12:18, Duan Fugang-B38611 wrote: > > On 06/26/13 18:09, Jim Baxter wrote: > >> > >>> In addition, > >>> 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. > >> I have only tested it on the i.MX6, if someone wants to check it on > >> the > >> imx28 later they can add it to that option. > > > >>> 2. Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user. > >> What do you mean by this, I am only enabling the VLAN code if > >> (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure > >> this is set by > > > > You can enable the feature like below, which is reasonable. > > .ndo_fix_features = fec_enet_fix_features > > > > static netdev_features_t fec_enet_fix_features (struct net_device *dev, > > netdev_features_t features) > > { > > struct fec_enet_private *fep = netdev_priv(dev); > > const struct platform_device_id *id_entry = > > platform_get_device_id(fep->pdev); > > > > if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) > > features |= NETIF_F_HW_VLAN_CTAG_RX; > > > > return features; > > } > > > > > > Thank you, > > Andy > > > > > > I currently set the features in fec_enet_init() along with setting hw_features, is fec_enet_fix_features() a more correct design for driver implementation point? Yes, I think it is more reasonable. Can you hold on the patch to hear others' idea. Thanks, Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/06/13 02:41, Duan Fugang-B38611 wrote: > On 06/26/13 19:45, Jim Baxter wrote: > >> On 26/06/13 12:18, Duan Fugang-B38611 wrote: >>> On 06/26/13 18:09, Jim Baxter wrote: >>>> >>>>> In addition, >>>>> 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. >>>> I have only tested it on the i.MX6, if someone wants to check it on >>>> the >>>> imx28 later they can add it to that option. >>> >>>>> 2. Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user. >>>> What do you mean by this, I am only enabling the VLAN code if >>>> (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure >>>> this is set by >>> >>> You can enable the feature like below, which is reasonable. >>> .ndo_fix_features = fec_enet_fix_features >>> >>> static netdev_features_t fec_enet_fix_features (struct net_device *dev, >>> netdev_features_t features) >>> { >>> struct fec_enet_private *fep = netdev_priv(dev); >>> const struct platform_device_id *id_entry = >>> platform_get_device_id(fep->pdev); >>> >>> if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) >>> features |= NETIF_F_HW_VLAN_CTAG_RX; >>> >>> return features; >>> } >>> >>> >>> Thank you, >>> Andy >>> >>> >> >> I currently set the features in fec_enet_init() along with setting hw_features, is fec_enet_fix_features() a more correct design for driver implementation point? > > Yes, I think it is more reasonable. > Can you hold on the patch to hear others' idea. > > Thanks, > Andy > > Yes, I am will wait for other comments. Thank you, Jim -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, den 26.06.2013, 12:45 +0100 schrieb Jim Baxter: > On 26/06/13 12:18, Duan Fugang-B38611 wrote: > > On 06/26/13 18:09, Jim Baxter wrote: > > > >>> In addition, > >>> 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. > >> I have only tested it on the i.MX6, if someone wants to check it on the > >> imx28 later they can add it to that option. > > > >>> 2. Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user. > >> What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by > > > > You can enable the feature like below, which is reasonable. > > .ndo_fix_features = fec_enet_fix_features > > > > static netdev_features_t fec_enet_fix_features (struct net_device *dev, > > netdev_features_t features) > > { > > struct fec_enet_private *fep = netdev_priv(dev); > > const struct platform_device_id *id_entry = > > platform_get_device_id(fep->pdev); > > > > if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) > > features |= NETIF_F_HW_VLAN_CTAG_RX; > > > > return features; > > } > > > > > > Thank you, > > Andy > > > > > > I currently set the features in fec_enet_init() along with setting > hw_features, is fec_enet_fix_features() a more correct design for driver > implementation point? > I agree that VLAN tag extraction should not be enabled by default, but at the users request. But according to the documentation there is no need to implement the fix_features callback for this to work. Just only set the hw_features on init, the networking core should take care of never trying to enable a feature not found in there. Regards, Lucas
On 27/06/13 10:44, Lucas Stach wrote: > Am Mittwoch, den 26.06.2013, 12:45 +0100 schrieb Jim Baxter: >> On 26/06/13 12:18, Duan Fugang-B38611 wrote: >>> On 06/26/13 18:09, Jim Baxter wrote: >>> >>>>> In addition, >>>>> 1. enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data. >>>> I have only tested it on the i.MX6, if someone wants to check it on the >>>> imx28 later they can add it to that option. >>> >>>>> 2. Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user. >>>> What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by >>> >>> You can enable the feature like below, which is reasonable. >>> .ndo_fix_features = fec_enet_fix_features >>> >>> static netdev_features_t fec_enet_fix_features (struct net_device *dev, >>> netdev_features_t features) >>> { >>> struct fec_enet_private *fep = netdev_priv(dev); >>> const struct platform_device_id *id_entry = >>> platform_get_device_id(fep->pdev); >>> >>> if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) >>> features |= NETIF_F_HW_VLAN_CTAG_RX; >>> >>> return features; >>> } >>> >>> >>> Thank you, >>> Andy >>> >>> >> >> I currently set the features in fec_enet_init() along with setting >> hw_features, is fec_enet_fix_features() a more correct design for driver >> implementation point? >> > I agree that VLAN tag extraction should not be enabled by default, but > at the users request. But according to the documentation there is no > need to implement the fix_features callback for this to work. > > Just only set the hw_features on init, the networking core should take > care of never trying to enable a feature not found in there. > > Regards, > Lucas > Hi Lucas, Yes that is how the patch currently operates, the user can still choose whether the feature is enabled. Thank you, Jim -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index e3ed6c5..85e75d9 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -148,6 +148,9 @@ struct bufdesc_ex { #define BD_ENET_RX_CL ((ushort)0x0001) #define BD_ENET_RX_STATS ((ushort)0x013f) /* All status bits */ +/* Enhanced buffer descriptor control/status used by Ethernet receive */ +#define BD_ENET_RX_VLAN 0x00000004 + /* Buffer descriptor control/status used by Ethernet transmit. */ #define BD_ENET_TX_READY ((ushort)0x8000) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 46f2544..851d836 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -54,6 +54,7 @@ #include <linux/of_gpio.h> #include <linux/of_net.h> #include <linux/regulator/consumer.h> +#include <linux/if_vlan.h> #include <asm/cacheflush.h> @@ -88,6 +89,8 @@ #define FEC_QUIRK_HAS_BUFDESC_EX (1 << 4) /* Controller has hardware checksum support */ #define FEC_QUIRK_HAS_CSUM (1 << 5) +/* Controller has hardware vlan support */ +#define FEC_QUIRK_HAS_VLAN (1 << 6) static struct platform_device_id fec_devtype[] = { { @@ -106,7 +109,8 @@ static struct platform_device_id fec_devtype[] = { }, { .name = "imx6q-fec", .driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT | - FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM, + FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM | + FEC_QUIRK_HAS_VLAN, }, { .name = "mvf600-fec", .driver_data = FEC_QUIRK_ENET_MAC, @@ -177,11 +181,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); #define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII) #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF)) -/* The FEC stores dest/src/type, data, and checksum for receive packets. +/* The FEC stores dest/src/type/vlan, data, and checksum for receive packets. */ -#define PKT_MAXBUF_SIZE 1518 +#define PKT_MAXBUF_SIZE 1522 #define PKT_MINBUF_SIZE 64 -#define PKT_MAXBLR_SIZE 1520 +#define PKT_MAXBLR_SIZE 1540 [Andy]Pls define the MAXBLR size to 1536, which is multiplied by 64 #define PKT_MAXBLR_SIZE 1536 /* FEC receive acceleration */ #define FEC_RACC_IPDIS (1 << 1) @@ -795,6 +799,9 @@ fec_enet_rx(struct net_device *ndev, int budget) ushort pkt_len; __u8 *data; int pkt_received = 0; + struct bufdesc_ex *ebdp = NULL; + bool vlan_packet_rcvd = false; + u16 vlan_tag; #ifdef CONFIG_M532x flush_cache_all(); @@ -858,6 +865,24 @@ fec_enet_rx(struct net_device *ndev, int budget) if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME) swap_buffer(data, pkt_len); + /* Extract the enhanced buffer descriptor */ + ebdp = NULL; + if (fep->bufdesc_ex) + 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) && + ebdp && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) { + /* Push and remove the vlan tag */ + struct vlan_hdr *vlan_header = + (struct vlan_hdr *) (data + ETH_HLEN); + vlan_tag = ntohs(vlan_header->h_vlan_TCI); + pkt_len -= VLAN_HLEN; + + vlan_packet_rcvd = true; + } + /* This does 16 byte alignment, exactly what we need. * The packet length includes FCS, but we don't want to * include that when passing upstream as it messes up @@ -868,18 +893,25 @@ fec_enet_rx(struct net_device *ndev, int budget)