Received: by 2002:a25:7ec1:0:0:0:0:0 with SMTP id z184csp1875175ybc; Wed, 13 Nov 2019 05:45:59 -0800 (PST) X-Google-Smtp-Source: APXvYqxi5HGlDExKABOMVa8BajysaXn33Jcl6rGMMQTJN3aprZx7+Jc3sA55yNZBhOzhafWCK2iA X-Received: by 2002:aa7:c39a:: with SMTP id k26mr3560767edq.128.1573652759130; Wed, 13 Nov 2019 05:45:59 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1573652759; cv=none; d=google.com; s=arc-20160816; b=1B9kw5jAEPGhG+pjd/l/d3KHnmCsi9RD3mhw4Y3OkYm2j3W160eyT1CDVwlwDd2NYO yN0LJfIzf96j2Sa6ZA6qd6QDlQkdLaHMySOc5jl41pnT8IoBlytdbCWsPnD+3ooC/uOB 36hOmB4NZ0evd+ShxqsfrAgDx/ThQQF5p+G/AwvvWiuRbxFPhuMuAPu8SBkkQfNzdJES UHgwFE9gA8aZEA9iPqijCKCedaAJNh7KnuDN0sYy5kE7Dwe1EoMcCRy69XN+dF11VSRi EeNc3ts51SzUmAI5m6TqF6bpS3bAFZX3civUnVlGK7l7F/yofuPDH34UaHOcWnGMqur0 8r7g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-transfer-encoding:content-disposition:mime-version :references:mail-followup-to:message-id:subject:cc:to:from:date :dkim-signature; bh=7VCmbEVddaGiCCPH6BV+lZcmBJTHDXU7QLX9bBBFD+o=; b=D5DKp9KcgkvS4G9nWYC14TNQdDjwHiDALnvx9KUr//Em6DhzAJoJgOQa8ZiFlaiipA DJnROmKFQoUfoEKdJPTGmG9VdLc73mvAL39inNFk4v0Q8LeuhrTCUc5wu4/KdvANeNRB VlYSo2ZF9zPzIPLD8vN+S3r+cUXviqUhdmgcm/WFWZGPL5JjFbFxjfLEFv39P3b0Ksxn 7qsua945DkcyvH23ERnAVLHhokEnpt0qE8urd/uLA27+SQwZtzSECRUjM1ct5+B4Yh51 nQlff20g9H0N67Bon7vC0278/4JNjcS/OJRs0947f2WvquSsPC8SmPo9Q0kZdM9my3PB 53ag== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=fLRGbGdd; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id b53si1441632edb.419.2019.11.13.05.45.34; Wed, 13 Nov 2019 05:45:59 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=fLRGbGdd; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727463AbfKMNlv (ORCPT + 99 others); Wed, 13 Nov 2019 08:41:51 -0500 Received: from mail-lf1-f67.google.com ([209.85.167.67]:40389 "EHLO mail-lf1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727137AbfKMNlu (ORCPT ); Wed, 13 Nov 2019 08:41:50 -0500 Received: by mail-lf1-f67.google.com with SMTP id j26so1992111lfh.7 for ; Wed, 13 Nov 2019 05:41:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=7VCmbEVddaGiCCPH6BV+lZcmBJTHDXU7QLX9bBBFD+o=; b=fLRGbGddTVjrMg7NvQKf2dOxVkisty4DDw/FMCI9gPqU66vAZlxSbO4xoEKpbyONE9 4P/kSYntUDOiM16n2T6v+yx7mJMvQnLPjg1jU1sNO4dy7CdNpkvQ5h6Cn7PLwhYb3WM1 zxCB+5oVkDxLum4MK7tzFw1yVjAf8pgXSMMvaeE5lYX49msnwbnzm1FF0AKqucq/v+wK cb5vn4/ed67OB84VqK1U+g3rCl/41VBZFU92VxfQYzSsZMPrbDKB16AvDELxEwxpyoXA 18NTiENwdpPVk8oj8z6hlHMejbAniWoqgPyJ+sV4sMSa7WWhbmsxrfESfR4UGdSr+14l k8rQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to:user-agent; bh=7VCmbEVddaGiCCPH6BV+lZcmBJTHDXU7QLX9bBBFD+o=; b=Mts5IougloxD54e5D2rbZ9s/t9bxkUPsfTWP8dBSs2cq1BUKrQKmCFQ3iShbVAHB/X zjgH3IUz4fp/m48InZu53ivi2FHQEVehUT6CdaekNeFWInWVcCFCv5si7H7qlCb+9sva PHeA1cj9U3ixevzhLy3SqiD4RW3LMGASooopqUu5tz5WnMI9u+JEf9p1JZ5lDYiMB9Kr a3hwFUK1oj/UFIPNtfzHobmQkJAGxO3k7wHgosf0FypPxzjRDZUrva/l2cvAlQe/ieVm 26ZK5H1mmpeqtJYyuSZcMid5WNNOzGUmM/Tzf2wrVqEBU9yBLTM4aVFQBZYObS0UN8Dt vQqQ== X-Gm-Message-State: APjAAAVRMkTligrJ7ZVvTDEP2uUR4mu8GXj3DuZYhR1Y6+9GS2Ltj6SG MJsszY/8GyTXAj/ZOI6bQn8sWQ== X-Received: by 2002:ac2:51b5:: with SMTP id f21mr2728379lfk.159.1573652507050; Wed, 13 Nov 2019 05:41:47 -0800 (PST) Received: from khorivan (57-201-94-178.pool.ukrtel.net. [178.94.201.57]) by smtp.gmail.com with ESMTPSA id n133sm1147966lfd.88.2019.11.13.05.41.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Nov 2019 05:41:46 -0800 (PST) Date: Wed, 13 Nov 2019 15:41:44 +0200 From: Ivan Khoronzhuk To: Po Liu Cc: Claudiu Manoil , "davem@davemloft.net" , "linux-kernel@vger.kernel.org" , "netdev@vger.kernel.org" , "vinicius.gomes@intel.com" , Vladimir Oltean , Alexandru Marginean , Xiaoliang Yang , Roy Zang , Mingkai Hu , Jerry Huang , Leo Li Subject: Re: [EXT] Re: [v2,net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload Message-ID: <20191113134142.GA2508@khorivan> Mail-Followup-To: Po Liu , Claudiu Manoil , "davem@davemloft.net" , "linux-kernel@vger.kernel.org" , "netdev@vger.kernel.org" , "vinicius.gomes@intel.com" , Vladimir Oltean , Alexandru Marginean , Xiaoliang Yang , Roy Zang , Mingkai Hu , Jerry Huang , Leo Li References: <20191111042715.13444-2-Po.Liu@nxp.com> <20191112082823.28998-1-Po.Liu@nxp.com> <20191112210958.GB1833@khorivan> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 13, 2019 at 03:45:08AM +0000, Po Liu wrote: >Hi Ivan, > >> -----Original Message----- >> From: Ivan Khoronzhuk >> Sent: 2019年11月13日 5:10 >> To: Po Liu >> Cc: Claudiu Manoil ; davem@davemloft.net; linux- >> kernel@vger.kernel.org; netdev@vger.kernel.org; vinicius.gomes@intel.com; >> Vladimir Oltean ; Alexandru Marginean >> ; Xiaoliang Yang >> ; Roy Zang ; Mingkai Hu >> ; Jerry Huang ; Leo Li >> >> Subject: [EXT] Re: [v2,net-next, 1/2] enetc: Configure the Time-Aware Scheduler >> via tc-taprio offload >> >> Caution: EXT Email >> >> Hello, >> >> On Tue, Nov 12, 2019 at 08:42:49AM +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 >> >Signed-off-by: Vladimir Oltean >> >Signed-off-by: Claudiu Manoil >> >--- >> >changes: >> >- 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. >> > >> > drivers/net/ethernet/freescale/enetc/Kconfig | 10 ++ >> > drivers/net/ethernet/freescale/enetc/Makefile | 1 + >> > 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 | 150 ++++++++++++++++-- >> > .../net/ethernet/freescale/enetc/enetc_qos.c | 130 +++++++++++++++ >> > 7 files changed, 300 insertions(+), 22 deletions(-) create mode 100644 >> > drivers/net/ethernet/freescale/enetc/enetc_qos.c >> > >> >> [...] >> >> > >> >@@ -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); >> Sorry didn't see v2, so i duplicate my question here: >> >> This patch is for taprio offload, I see that mqprio is related and is part of taprio >> offload configuration. But taprio offload has own mqprio settings. >> The taprio mqprio part is not offloaded with TC_SETUP_QDISC_MQPRIO. >> >> So, a combination of mqprio and tario qdiscs used. >> Could you please share the commands were used for your setup? >> > >Example command: >tc qdisc replace dev eth0 parent root handle 100 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 01 sched-entry S 02 300000 flags 0x2 So, the TC_SETUP_QDISC_MQPRIO is really not required here, and mqprio qdisc is not used. Then why is it here, should be placed in separate patch at least. But even the comb mqprio qdisc and taprio qdisc are used together then taprio requires hw offload also. I'm Ok to add it later to taprio, and I'm asking about it because I need it also, what ever way it could be added. Not clear how you combined mqprio qdisc and taprio now to get it working From this command: tc qdisc replace dev eth0 parent root handle 100 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 01 sched-entry S 02 300000 flags 0x2 I can conclude that some default h/w mapping (one to one) were used and no need to use mqprio qdisc, the command above is enough. Is it true? Then move it to spearate patch please as it's not taprio related yet. > >> And couple interesting questions about all of this: >> - The taprio qdisc has to have mqprio settings, but if it's done with mqprio then >> it just skipped (by reading tc class num). >> - If no separate mqprio qdisc configuration then mqprio conf from taprio is set, >> who should restore tc mappings when taprio qdisc is unloaded? >> Maybe there is reason to implement TC_SETUP_QDISC_MQPRIO offload in >> taprio since it's required feature? > [...] >> >> >+ >> >+ /* Configure the (administrative) gate control list using the >> >+ * control BD descriptor. >> >+ */ >> >+ gcl_config = &cbd.gcl_conf; >> >+ >> >+ data_size = sizeof(struct tgs_gcl_data) + gcl_len * >> >+ sizeof(struct gce); >> >+ >> >+ gcl_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL); >> >+ if (!gcl_data) >> >+ return -ENOMEM; >> >+ >> >+ gce = (struct gce *)(gcl_data + 1); >> >+ >> >+ /* Since no initial state config in taprio, set gates open as default. >> >+ */ >> tc-taprio and IEEE Qbv allows to change configuration in flight, so that oper >> state is active till new admin start time. So, here comment says it does initial >> state config, if in-flight feature is not supported then error has to be returned >> instead of silently rewriting configuration. But if it can be implemented then >> state should be remembered/verified in order to not brake oper configuration? > >I think this is ok as per standard. Also see this comment in >net/sched/sch_taprio.c: From the code above (duplicate for convenience): if (admin_conf->enable) { enetc_wr(&priv->si->hw, ENETC_QBV_PTGCR_OFFSET, temp & (~ENETC_QBV_TGE)); usleep_range(10, 20); enetc_wr(&priv->si->hw, ENETC_QBV_PTGCR_OFFSET, temp | ENETC_QBV_TGE); } else { enetc_wr(&priv->si->hw, ENETC_QBV_PTGCR_OFFSET, temp & (~ENETC_QBV_TGE)); return 0; } I see that's not true, as Simon noted, you have same command: enetc_wr(&priv->si->hw, ENETC_QBV_PTGCR_OFFSET, temp & (~ENETC_QBV_TGE)); before enabling and for disabling. I guess it's stop command, that is disable qbv. So, before enabling new configuration with enetc_setup_tc(), the tario is inited|cleared|reseted|rebooted|defaulted|offed but not updated. It means no in-flight capabilities or they are ignored. JFI, it's possible to do first time: tc qdisc replace dev eth0 parent root handle 100 taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 01 sched-entry S 02 300000 flags 0x2 and then: tc qdisc replace dev eth0 parent root handle 100 taprio base-time 01 sched-entry S 02 200000 flags 0x2 Do it many times, but w/o mqprio configuration. So, this function must return error if it cannot be done, as above commands suppose that configuration can be updated in runtime, that is, set ADMIN cycle while OPER cycle is still active for some time and not broken like it's done now. If it can be achieved then no need to do enetc_wr(&priv->si->hw, ENETC_QBV_PTGCR_OFFSET, temp & (~ENETC_QBV_TGE)); when admin_conf->enable. So or return error, or do it appropriately. > > /* Until the schedule starts, all the queues are open */ >I would change the comment. > >> >+ gcl_config->atc = 0xff; >> >+ gcl_config->acl_len = cpu_to_le16(gcl_len); >> >> Ok, this is maximum number of schedules. >> According to tc-taprio it's possible to set cycle period more then schedules >> actually can consume. If cycle time is more, then last gate's state can be kept >> till the end of cycle. But if last schedule has it's own interval set then gates >> should be closed till the end of cycle or no? if it has to be closed, then one more >> endl schedule should be present closing gates at the end of list for the rest cycle >> time. Can be implemented in h/w but just to be sure, how it's done in h/w? >> >There is already check the list len in up code. >if (admin_conf->num_entries > enetc_get_max_gcl_len(&priv->si->hw)) > return -EINVAL; >gcl_len = admin_conf->num_entries; I mean +1 schedule to finalize the cycle with closed gates if last schedule has provided time interval. If I set couple schedules, with intervals, and cycle time more then those schedules consume, then I suppose the gates closed for the rest time of cycle, probably. Example: sched1 ----> shced2 ----> time w/o scheds -> cycle end gate 1 gate 2 no gates But if shced2 is last one then gate 2 is opened till the end of cycle. So, to close gate one more shched is needed with closed gates to finalize it. Or it's supposed that gate2 is opened till the end of cycle, How is it in you case. Or this is can be provided by configuration from tc? It's question not statement. Just though. Anyway i'll verify later it can be done with tc and how it's done in sw version, just interesting how your h/w works. > >> >+ >> >+ 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)); [...] -- Regards, Ivan Khoronzhuk