Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
From:   rohan.g.thomas@intel.com
To:     robh@kernel.org
Cc:     alexandre.torgue@foss.st.com, conor+dt@kernel.org,
        davem@davemloft.net, devicetree@vger.kernel.org,
        edumazet@google.com, fancer.lancer@gmail.com, joabreu@synopsys.com,
        krzysztof.kozlowski+dt@linaro.org, kuba@kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        mcoquelin.stm32@gmail.com, netdev@vger.kernel.org,
        pabeni@redhat.com, peppe.cavallaro@st.com, rohan.g.thomas@intel.com
Subject: Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: Time Based Scheduling
Date:   Fri, 29 Sep 2023 13:17:58 +0800
Message-Id: <20230929051758.21492-1-rohan.g.thomas@intel.com>
In-Reply-To: <20230928180942.GA932326-robh@kernel.org>
References: <20230928180942.GA932326-robh@kernel.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Rohan G Thomas <rohan.g.thomas@intel.com>

On Wed, Sep 27, 2023 at 09:09:18PM +0800, Rohan G Thomas wrote:
>> Add new property tbs-enabled to enable Time Based Scheduling(TBS)
>
>That's not the property you added.
>
>> support per Tx queues. TBS feature can be enabled later using ETF
>> qdisc but for only those queues that have TBS support enabled.
>
>This property defines capable or enabled? 

This property is to enable TBS support for any Tx queue. Why this
added is because I think TBS need not be enabled for all capable
Tx queues(Tx DMA channels) because of the following hw limitations.
1. As per DWMAC QoS and DWXGMAC databooks, TBS cannot coexist with
TSO. So TBS cannot be enabled for a Tx queue which is for TSO. 
2. Also as per DWXGMAC databook, "Do not enable time-based scheduling
(or enhanced descriptors) for the channel for which TSO or transmit
timestamp or one-step timestamping control correction feature is
enabled".
3. As per DWXGMAC databook, "Use separate channel (without TBS
enabled) for time critical traffic. Mixing such traffic with TBS
enabled traffic can cause delays in transmitting time critical
traffic."
More explanation below...

>
>Seems like OS configuration and policy.

Tx queues need to be configured for TBS during hw setup itself as
special enhanced descriptors are used by the hw for TBS support
enabled queues. Switching between enhanced and normal descriptors
on run is not feasible. So this flag is for enabling "Enhanced
Descriptors for Time Based Scheduling". This I think is a hw specific
requirement.

>
>Doesn't eh DWMAC have capability registers for supported features? Or 
>did they forget per queue capabilities?

Yes, capability registers are available. For DWMAC5 IP, if TBSSEL bit
is set, then TBS is supported by all Tx queues. For DWXGMAC IP, if
TBSSEL bit is set, then TBS is supported by TBS_CH number of Tx
queues starting from the highest Tx queue. But because of the hw
limitations mentioned above, TBS cannot be enabled for all capable
queues.

>
>> 
>> Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
>> entry") enables similar support from the stmmac pci driver.
>
>Why does unconditionally enabling TBS work there, but not here?

There, Tx queue 0 is not enabled for TBS as it is used for TSO.

>
>> 
>> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
>> ---
>>  Documentation/devicetree/bindings/net/snps,dwmac.yaml | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/>devicetree/bindings/net/snps,dwmac.yaml
>> index 5c2769dc689a..db1eb0997602 100644
>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> @@ -399,6 +399,14 @@ properties:
>>              type: boolean
>>              description: TX checksum offload is unsupported by the TX queue.
>>  
>> +          snps,tbs-enabled:
>> +            type: boolean
>> +            description:
>> +              Enable Time Based Scheduling(TBS) support for the TX queue. TSO and
>> +              TBS cannot be supported by a queue at the same time. If TSO support
>> +              is enabled, then default TX queue 0 for TSO and in that case don't
>> +              enable TX queue 0 for TBS.
>> +
>>          allOf:
>>            - if:
>>                required:
>> -- 
>> 2.26.2
>>