Date:   Fri, 15 Nov 2019 02:37:58 +0000
From:   Po Liu <po.liu@....com>
To:     Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
CC:     Claudiu Manoil <claudiu.manoil@....com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "vinicius.gomes@...el.com" <vinicius.gomes@...el.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Roy Zang <roy.zang@....com>, Mingkai Hu <mingkai.hu@....com>,
        Jerry Huang <jerry.huang@....com>, Leo Li <leoyang.li@....com>
Subject: RE: [EXT] Re: [v3,net-next, 1/2] enetc: Configure the Time-Aware
 Scheduler via tc-taprio offload




Br,
Po Liu

> -----Original Message-----
> From: Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
> Sent: 2019年11月15日 2:51
> To: Po Liu <po.liu@....com>
> Cc: Claudiu Manoil <claudiu.manoil@....com>; davem@...emloft.net; linux-
> kernel@...r.kernel.org; netdev@...r.kernel.org; vinicius.gomes@...el.com;
> Vladimir Oltean <vladimir.oltean@....com>; Alexandru Marginean
> <alexandru.marginean@....com>; Xiaoliang Yang
> <xiaoliang.yang_1@....com>; Roy Zang <roy.zang@....com>; Mingkai Hu
> <mingkai.hu@....com>; Jerry Huang <jerry.huang@....com>; Leo Li
> <leoyang.li@....com>
> 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@....com>
> >Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> >Signed-off-by: Claudiu Manoil <claudiu.manoil@....com>
> >---
> >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

Powered by blists - more mailing lists