RE: [EXT] Re: [v3,net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

From: Po Liu
Date: Thu Nov 14 2019 - 21:38:20 EST




Br,
Po Liu

> -----Original Message-----
> From: Ivan Khoronzhuk <ivan.khoronzhuk@xxxxxxxxxx>
> Sent: 2019年11月15日 2:51
> To: Po Liu <po.liu@xxxxxxx>
> Cc: Claudiu Manoil <claudiu.manoil@xxxxxxx>; davem@xxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; vinicius.gomes@xxxxxxxxx;
> Vladimir Oltean <vladimir.oltean@xxxxxxx>; Alexandru Marginean
> <alexandru.marginean@xxxxxxx>; Xiaoliang Yang
> <xiaoliang.yang_1@xxxxxxx>; Roy Zang <roy.zang@xxxxxxx>; Mingkai Hu
> <mingkai.hu@xxxxxxx>; Jerry Huang <jerry.huang@xxxxxxx>; Leo Li
> <leoyang.li@xxxxxxx>
> Subject: [EXT] Re: [v3,net-next, 1/2] enetc: Configure the Time-Aware Scheduler
> via tc-taprio offload
>
> Caution: EXT Email
>
> On Thu, Nov 14, 2019 at 05:12:30AM +0000, Po Liu wrote:
> >ENETC supports in hardware for time-based egress shaping according to
> >IEEE 802.1Qbv. This patch implement the Qbv enablement by the hardware
> >offload method qdisc tc-taprio method.
> >Also update cbdr writeback to up level since control bd ring may
> >writeback data to control bd ring.
> >
> >Signed-off-by: Po Liu <Po.Liu@xxxxxxx>
> >Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> >Signed-off-by: Claudiu Manoil <claudiu.manoil@xxxxxxx>
> >---
> >changes:
> >v2:
> >- introduce a local define CONFIG_FSL_ENETC_QOS to fix the various
> > configurations will result in link errors.
> > Since the CONFIG_NET_SCH_TAPRIO depends on many Qos configs. Not
> > to use it directly in driver. Add it to CONFIG_FSL_ENETC_QOS depends
> > on list, so only CONFIG_NET_SCH_TAPRIO enabled, user can enable this
> > tsn feature, or else, return not support.
> >v3:
> >- fix the compiling vf module failure issue:
> > ERROR: "enetc_sched_speed_set" [drivers/net/ethernet/freescale/enetc/fsl-
> enetc-vf.ko] undefined!
> > ERROR: "enetc_setup_tc_taprio" [drivers/net/ethernet/freescale/enetc/fsl-
> enetc-vf.ko] undefined!
> >- remove defines not used in this patch
> >- fix hardware endian issue
> >- make the qbv set code more rubust with some error condition may occur.
> >
> > drivers/net/ethernet/freescale/enetc/Kconfig | 10 ++
> > drivers/net/ethernet/freescale/enetc/Makefile | 2 +
> > drivers/net/ethernet/freescale/enetc/enetc.c | 19 ++-
> > drivers/net/ethernet/freescale/enetc/enetc.h | 7 +
> > .../net/ethernet/freescale/enetc/enetc_cbdr.c | 5 +-
> > .../net/ethernet/freescale/enetc/enetc_hw.h | 84 ++++++++---
> > .../net/ethernet/freescale/enetc/enetc_qos.c | 134 ++++++++++++++++++
> > 7 files changed, 239 insertions(+), 22 deletions(-) create mode 100644
> > drivers/net/ethernet/freescale/enetc/enetc_qos.c
> >
> >diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig
> >b/drivers/net/ethernet/freescale/enetc/Kconfig
> >index c219587bd334..491659fe3e35 100644
> >--- a/drivers/net/ethernet/freescale/enetc/Kconfig
> >+++ b/drivers/net/ethernet/freescale/enetc/Kconfig
> >@@ -50,3 +50,13 @@ config FSL_ENETC_HW_TIMESTAMPING
> > allocation has not been supported and it is too expensive to use
> > extended RX BDs if timestamping is not used, this option enables
> > extended RX BDs in order to support hardware timestamping.
> >+
> >+config FSL_ENETC_QOS
> >+ bool "ENETC hardware Time-sensitive Network support"
> >+ depends on (FSL_ENETC || FSL_ENETC_VF) && NET_SCH_TAPRIO
> >+ help
> >+ There are Time-Sensitive Network(TSN) capabilities(802.1Qbv/802.1Qci
> >+ /802.1Qbu etc.) supported by ENETC. These TSN capabilities can be set
> >+ enable/disable from user space via Qos commands(tc). In the kernel
> >+ side, it can be loaded by Qos driver. Currently, it is only support
> >+ taprio(802.1Qbv).
> >diff --git a/drivers/net/ethernet/freescale/enetc/Makefile
> >b/drivers/net/ethernet/freescale/enetc/Makefile
> >index d200c27c3bf6..d0db33e5b6b7 100644
> >--- a/drivers/net/ethernet/freescale/enetc/Makefile
> >+++ b/drivers/net/ethernet/freescale/enetc/Makefile
> >@@ -5,9 +5,11 @@ common-objs := enetc.o enetc_cbdr.o enetc_ethtool.o
> > obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o fsl-enetc-y := enetc_pf.o
> >enetc_mdio.o $(common-objs)
> > fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
> >+fsl-enetc-$(CONFIG_FSL_ENETC_QOS) += enetc_qos.o
> >
> > obj-$(CONFIG_FSL_ENETC_VF) += fsl-enetc-vf.o fsl-enetc-vf-y :=
> > enetc_vf.o $(common-objs)
> >+fsl-enetc-vf-$(CONFIG_FSL_ENETC_QOS) += enetc_qos.o
> >
> > obj-$(CONFIG_FSL_ENETC_MDIO) += fsl-enetc-mdio.o fsl-enetc-mdio-y :=
> >enetc_pci_mdio.o enetc_mdio.o diff --git
> >a/drivers/net/ethernet/freescale/enetc/enetc.c
> >b/drivers/net/ethernet/freescale/enetc/enetc.c
> >index 3e8f9819f08c..d58dbc2c4270 100644
> >--- a/drivers/net/ethernet/freescale/enetc/enetc.c
> >+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> >@@ -1427,8 +1427,7 @@ int enetc_close(struct net_device *ndev)
> > return 0;
> > }
> >
> >-int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> >- void *type_data)
> >+int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
> > {
> > struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > struct tc_mqprio_qopt *mqprio = type_data; @@ -1436,9 +1435,6 @@
> >int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> > u8 num_tc;
> > int i;
> >
> >- if (type != TC_SETUP_QDISC_MQPRIO)
> >- return -EOPNOTSUPP;
> >-
> > mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
> > num_tc = mqprio->num_tc;
> >
> >@@ -1483,6 +1479,19 @@ int enetc_setup_tc(struct net_device *ndev, enum
> tc_setup_type type,
> > return 0;
> > }
> >
> >+int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> >+ void *type_data)
> >+{
> >+ switch (type) {
> >+ case TC_SETUP_QDISC_MQPRIO:
> >+ return enetc_setup_tc_mqprio(ndev, type_data);
> >+ case TC_SETUP_QDISC_TAPRIO:
> >+ return enetc_setup_tc_taprio(ndev, type_data);
> >+ default:
> >+ return -EOPNOTSUPP;
> >+ }
> >+}
> >+
> > struct net_device_stats *enetc_get_stats(struct net_device *ndev) {
> > struct enetc_ndev_priv *priv = netdev_priv(ndev); diff --git
> >a/drivers/net/ethernet/freescale/enetc/enetc.h
> >b/drivers/net/ethernet/freescale/enetc/enetc.h
> >index 541b4e2073fe..8ca2f97050c8 100644
> >--- a/drivers/net/ethernet/freescale/enetc/enetc.h
> >+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> >@@ -244,3 +244,10 @@ int enetc_set_fs_entry(struct enetc_si *si, struct
> >enetc_cmd_rfse *rfse, void enetc_set_rss_key(struct enetc_hw *hw,
> >const u8 *bytes); int enetc_get_rss_table(struct enetc_si *si, u32
> >*table, int count); int enetc_set_rss_table(struct enetc_si *si, const
> >u32 *table, int count);
> >+int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd);
> >+
> >+#ifdef CONFIG_FSL_ENETC_QOS
> >+int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data);
> >+#else #define enetc_setup_tc_taprio(ndev, type_data) -EOPNOTSUPP
> >+#endif
> >diff --git a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> >b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> >index de466b71bf8f..201cbc362e33 100644
> >--- a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> >+++ b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> >@@ -32,7 +32,7 @@ static int enetc_cbd_unused(struct enetc_cbdr *r)
> > r->bd_count;
> > }
> >
> >-static int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> >+int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> > {
> > struct enetc_cbdr *ring = &si->cbd_ring;
> > int timeout = ENETC_CBDR_TIMEOUT; @@ -66,6 +66,9 @@ static int
> >enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> > if (!timeout)
> > return -EBUSY;
> >
> >+ /* CBD may writeback data, feedback up level */
> >+ *cbd = *dest_cbd;
> >+
> > enetc_clean_cbdr(si);
> >
> > return 0;
> >diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> >b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> >index 88276299f447..df6b35dc3534 100644
> >--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> >+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> >@@ -18,6 +18,7 @@
> > #define ENETC_SICTR0 0x18
> > #define ENETC_SICTR1 0x1c
> > #define ENETC_SIPCAPR0 0x20
> >+#define ENETC_SIPCAPR0_QBV BIT(4)
> > #define ENETC_SIPCAPR0_RSS BIT(8)
> > #define ENETC_SIPCAPR1 0x24
> > #define ENETC_SITGTGR 0x30
> >@@ -440,22 +441,6 @@ union enetc_rx_bd {
> > #define EMETC_MAC_ADDR_FILT_RES 3 /* # of reserved entries at the
> beginning */
> > #define ENETC_MAX_NUM_VFS 2
> >
> >-struct enetc_cbd {
> >- union {
> >- struct {
> >- __le32 addr[2];
> >- __le32 opt[4];
> >- };
> >- __le32 data[6];
> >- };
> >- __le16 index;
> >- __le16 length;
> >- u8 cmd;
> >- u8 cls;
> >- u8 _res;
> >- u8 status_flags;
> >-};
> >-
> > #define ENETC_CBD_FLAGS_SF BIT(7) /* short format */
> > #define ENETC_CBD_STATUS_MASK 0xf
> >
> >@@ -554,3 +539,70 @@ static inline void enetc_set_bdr_prio(struct
> enetc_hw *hw, int bdr_idx,
> > val |= ENETC_TBMR_SET_PRIO(prio);
> > enetc_txbdr_wr(hw, bdr_idx, ENETC_TBMR, val); }
> >+
> >+enum bdcr_cmd_class {
> >+ BDCR_CMD_UNSPEC = 0,
> >+ BDCR_CMD_MAC_FILTER,
> >+ BDCR_CMD_VLAN_FILTER,
> >+ BDCR_CMD_RSS,
> >+ BDCR_CMD_RFS,
> >+ BDCR_CMD_PORT_GCL,
> >+ BDCR_CMD_RECV_CLASSIFIER,
> >+ __BDCR_CMD_MAX_LEN,
> >+ BDCR_CMD_MAX_LEN = __BDCR_CMD_MAX_LEN - 1, };
> >+
> >+/* class 5, command 0 */
> >+struct tgs_gcl_conf {
> >+ u8 atc; /* init gate value */
> >+ u8 res[7];
> >+ struct {
> >+ u8 res1[4];
> >+ __le16 acl_len;
> >+ u8 res2[2];
> >+ };
> >+};
> >+
> >+/* gate control list entry */
> >+struct gce {
> >+ __le32 period;
> >+ u8 gate;
> >+ u8 res[3];
> >+};
> >+
> >+/* tgs_gcl_conf address point to this data space */ struct
> >+tgs_gcl_data {
> >+ __le32 btl;
> >+ __le32 bth;
> >+ __le32 ct;
> >+ __le32 cte;
> >+ struct gce entry[0];
> >+};
> >+
> >+struct enetc_cbd {
> >+ union{
> >+ struct {
> >+ __le32 addr[2];
> >+ union {
> >+ __le32 opt[4];
> >+ struct tgs_gcl_conf gcl_conf;
> >+ };
> >+ }; /* Long format */
> >+ __le32 data[6];
> >+ };
> >+ __le16 index;
> >+ __le16 length;
> >+ u8 cmd;
> >+ u8 cls;
> >+ u8 _res;
> >+ u8 status_flags;
> >+};
> >+
> >+/* port time gating control register */
> >+#define ENETC_QBV_PTGCR_OFFSET 0x11a00
> >+#define ENETC_QBV_TGE BIT(31)
> >+#define ENETC_QBV_TGPE BIT(30)
> >+
> >+/* Port time gating capability register */
> >+#define ENETC_QBV_PTGCAPR_OFFSET 0x11a08
> >+#define ENETC_QBV_MAX_GCL_LEN_MASK GENMASK(15, 0)
> >diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> >b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> >new file mode 100644
> >index 000000000000..9ce983c00201
> >--- /dev/null
> >+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> >@@ -0,0 +1,134 @@
> >+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> >+/* Copyright 2019 NXP */
> >+
> >+#include "enetc.h"
> >+
> >+#include <net/pkt_sched.h>
> >+
> >+static u16 enetc_get_max_gcl_len(struct enetc_hw *hw) {
> >+ return enetc_rd(hw, ENETC_QBV_PTGCAPR_OFFSET)
> >+ & ENETC_QBV_MAX_GCL_LEN_MASK; }
> >+
> >+static int enetc_setup_taprio(struct net_device *ndev,
> >+ struct tc_taprio_qopt_offload *admin_conf)
> >+{
> >+ struct enetc_ndev_priv *priv = netdev_priv(ndev);
> >+ struct enetc_cbd cbd = {.cmd = 0};
> >+ struct tgs_gcl_conf *gcl_config;
> >+ struct tgs_gcl_data *gcl_data;
> >+ struct gce *gce;
> >+ dma_addr_t dma;
> >+ u16 data_size;
> >+ u16 gcl_len;
> >+ u32 tge;
> >+ int err;
> >+ int i;
> >+
> >+ if (admin_conf->num_entries > enetc_get_max_gcl_len(&priv->si->hw))
> >+ return -EINVAL;
> >+ gcl_len = admin_conf->num_entries;
> >+
> >+ tge = enetc_rd(&priv->si->hw, ENETC_QBV_PTGCR_OFFSET);
> >+ if (!admin_conf->enable) {
> >+ enetc_wr(&priv->si->hw,
> >+ ENETC_QBV_PTGCR_OFFSET,
> >+ tge & (~ENETC_QBV_TGE));
> >+ return 0;
> >+ }
> >+
> >+ if (admin_conf->cycle_time > (u32)~0 ||
> >+ admin_conf->cycle_time_extension > (u32)~0)
> >+ return -EINVAL;
>
> U32_MAX?
>
> >+
> >+ /* Configure the (administrative) gate control list using the
> >+ * control BD descriptor.
> >+ */
> >+ gcl_config = &cbd.gcl_conf;
> >+
> >+ data_size = struct_size(gcl_data, entry, gcl_len);
> >+ gcl_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL);
> >+ if (!gcl_data)
> >+ return -ENOMEM;
> >+
> >+ gce = (struct gce *)(gcl_data + 1);
> >+
> >+ /* Set all gates open as default */
> >+ gcl_config->atc = 0xff;
> >+ gcl_config->acl_len = cpu_to_le16(gcl_len);
> >+
> >+ if (!admin_conf->base_time) {
> >+ gcl_data->btl =
> >+ cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR0));
> >+ gcl_data->bth =
> >+ cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR1));
> >+ } else {
> >+ gcl_data->btl =
> >+ cpu_to_le32(lower_32_bits(admin_conf->base_time));
> >+ gcl_data->bth =
> >+ cpu_to_le32(upper_32_bits(admin_conf->base_time));
> >+ }
> >+
> >+ gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
> >+ gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
> >+
> >+ for (i = 0; i < gcl_len; i++) {
> >+ struct tc_taprio_sched_entry *temp_entry;
> >+ struct gce *temp_gce = gce + i;
> >+
> >+ temp_entry = &admin_conf->entries[i];
> >+
> >+ temp_gce->gate = (u8)temp_entry->gate_mask;
> >+ temp_gce->period = cpu_to_le32(temp_entry->interval);
> >+ }
> >+
> >+ cbd.length = cpu_to_le16(data_size);
> >+ cbd.status_flags = 0;
> >+
> >+ dma = dma_map_single(&priv->si->pdev->dev, gcl_data,
> >+ data_size, DMA_TO_DEVICE);
> >+ if (dma_mapping_error(&priv->si->pdev->dev, dma)) {
> >+ netdev_err(priv->si->ndev, "DMA mapping failed!\n");
> >+ kfree(gcl_data);
> >+ return -ENOMEM;
> >+ }
> >+
> >+ cbd.addr[0] = lower_32_bits(dma);
> >+ cbd.addr[1] = upper_32_bits(dma);
> >+ cbd.cls = BDCR_CMD_PORT_GCL;
> >+ cbd.status_flags = 0;
> >+
> >+ enetc_wr(&priv->si->hw, ENETC_QBV_PTGCR_OFFSET,
> >+ tge & (~ENETC_QBV_TGE));
>
> Sorry, but, please, before sending new version, be sure you understand what
> should be changed.
>
> Let's back to IEEE Std 802.1Q
>
> Do you know what means cycle time extension that is programmed here.
> And why needed admin and oper configurations, and their base times?
>
> It's needed to not stop active oper configuration (that actually this function do),
> so that it's active till new one, that is admin, will be able to replace it.
>
> If user wants to stop it, and then load new one, he has to do it himself. But,
> when some configuration is active - that is OPER at the moment - and user
> requests w/o stopping it new configuration in some new admin base time, the
> oper stay active till this admin base time, then dropped in hw.
>
> What this code do, it's in hidden way, stop oper configuration at the time of
> configuration and no matter which admit base time is.

I am definitely know the spec in the Qbv. Here is hardware errata which I have mentioned in previous feedback in v2.

>
> That's definitely wrong. Just look how sw version works.
> And why all this admin/oper headache exists in IEEE Std 802.1Q exactly for this
> reason.
>
> So, if oper configuration exists at the moment of calling taprio configuration
> and no way to apply new on in some future time (admin base time), the code
> should say to user:
>
> Please, stop prev. oper configuration - then prog new one.
>
> The taprio interface is common and can't be treated differently depending on
> hw.
>
> Hope it's clean.
>
> >+
> >+ usleep_range(10, 20);
> >+
> >+ enetc_wr(&priv->si->hw, ENETC_QBV_PTGCR_OFFSET,
> >+ tge | ENETC_QBV_TGE);
> >+
> >+ err = enetc_send_cmd(priv->si, &cbd);
> >+ if (err)
> >+ enetc_wr(&priv->si->hw,
> >+ ENETC_QBV_PTGCR_OFFSET,
> >+ tge & (~ENETC_QBV_TGE));
> >+
> >+ dma_unmap_single(&priv->si->pdev->dev, dma, data_size,
> DMA_TO_DEVICE);
> >+ kfree(gcl_data);
> >+
> >+ return err;
> >+}
> >+
> >+int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data) {
> >+ struct tc_taprio_qopt_offload *taprio = type_data;
> >+ struct enetc_ndev_priv *priv = netdev_priv(ndev);
> >+ int i;
> >+
> >+ for (i = 0; i < priv->num_tx_rings; i++)
> >+ enetc_set_bdr_prio(&priv->si->hw,
> >+ priv->tx_ring[i]->index,
> >+ taprio->enable ? i : 0);
>
> Again, you should restore enetc_set_bdr_prio if below enetc_setup_taprio()
> fails.
>
> >+
> >+ return enetc_setup_taprio(ndev, taprio); }
> >--
> >2.17.1
> >
>
> --
> Regards,
> Ivan Khoronzhuk