| Message ID | 20220509010847.17492-1-muhammad.husaini.zulkifli@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | igc: Correct the launchtime offset | expand |
Hi Muhammad,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tnguy-next-queue/dev-queue]
[also build test WARNING on v5.18-rc6 next-20220509]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Muhammad-Husaini-Zulkifli/igc-Correct-the-launchtime-offset/20220509-091028
base: https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git dev-queue
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20220510/202205101357.46oBdnm7-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 3abb68a626160e019c30a4860e569d7bc75e486a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/819e4648eaccdeaf40643fd0fe7cd735a584c251
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Muhammad-Husaini-Zulkifli/igc-Correct-the-launchtime-offset/20220509-091028
git checkout 819e4648eaccdeaf40643fd0fe7cd735a584c251
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/kernel/ drivers/net/ethernet/intel/igc/ drivers/rtc/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/net/ethernet/intel/igc/igc_tsn.c:102:2: warning: variable 'tx_offset' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
default:
^~~~~~~
drivers/net/ethernet/intel/igc/igc_tsn.c:106:22: note: uninitialized use occurs here
wr32(IGC_GTXOFFSET, tx_offset);
^~~~~~~~~
drivers/net/ethernet/intel/igc/igc_regs.h:310:10: note: expanded from macro 'wr32'
writel((val), &hw_addr[(reg)]); \
^~~
arch/arm64/include/asm/io.h:142:52: note: expanded from macro 'writel'
#define writel(v,c) ({ __iowmb(); writel_relaxed((v),(c)); })
^
arch/arm64/include/asm/io.h:127:74: note: expanded from macro 'writel_relaxed'
#define writel_relaxed(v,c) ((void)__raw_writel((__force u32)cpu_to_le32(v),(c)))
^
include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__cpu_to_le32'
#define __cpu_to_le32(x) ((__force __le32)(__u32)(x))
^
drivers/net/ethernet/intel/igc/igc_tsn.c:87:15: note: initialize the variable 'tx_offset' to silence this warning
u16 tx_offset;
^
= 0
1 warning generated.
vim +/tx_offset +102 drivers/net/ethernet/intel/igc/igc_tsn.c
81
82 static int igc_tsn_enable_offload(struct igc_adapter *adapter)
83 {
84 struct igc_hw *hw = &adapter->hw;
85 u32 tqavctrl, baset_l, baset_h;
86 u32 sec, nsec, cycle;
87 u16 tx_offset;
88 ktime_t base_time, systim;
89 int i;
90
91 cycle = adapter->cycle_time;
92 base_time = adapter->base_time;
93
94 switch (adapter->link_speed) {
95 /* TODO: This code use same transmit offset across all link speed as for now. */
96 case SPEED_10:
97 case SPEED_100:
98 case SPEED_1000:
99 case SPEED_2500:
100 tx_offset = 1500;
101 break;
> 102 default:
103 break;
104 }
105
106 wr32(IGC_GTXOFFSET, tx_offset);
107 wr32(IGC_TSAUXC, 0);
108 wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_TSN);
109 wr32(IGC_TXPBS, IGC_TXPBSIZE_TSN);
110
111 tqavctrl = rd32(IGC_TQAVCTRL);
112 tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV;
113 wr32(IGC_TQAVCTRL, tqavctrl);
114
115 wr32(IGC_QBVCYCLET_S, cycle);
116 wr32(IGC_QBVCYCLET, cycle);
117
118 for (i = 0; i < adapter->num_tx_queues; i++) {
119 struct igc_ring *ring = adapter->tx_ring[i];
120 u32 txqctl = 0;
121 u16 cbs_value;
122 u32 tqavcc;
123
124 wr32(IGC_STQT(i), ring->start_time);
125 wr32(IGC_ENDQT(i), ring->end_time);
126
127 if (adapter->base_time) {
128 /* If we have a base_time we are in "taprio"
129 * mode and we need to be strict about the
130 * cycles: only transmit a packet if it can be
131 * completed during that cycle.
132 */
133 txqctl |= IGC_TXQCTL_STRICT_CYCLE |
134 IGC_TXQCTL_STRICT_END;
135 }
136
137 if (ring->launchtime_enable)
138 txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
139
140 /* Skip configuring CBS for Q2 and Q3 */
141 if (i > 1)
142 goto skip_cbs;
143
144 if (ring->cbs_enable) {
145 if (i == 0)
146 txqctl |= IGC_TXQCTL_QAV_SEL_CBS0;
147 else
148 txqctl |= IGC_TXQCTL_QAV_SEL_CBS1;
149
150 /* According to i225 datasheet section 7.5.2.7, we
151 * should set the 'idleSlope' field from TQAVCC
152 * register following the equation:
153 *
154 * value = link-speed 0x7736 * BW * 0.2
155 * ---------- * ----------------- (E1)
156 * 100Mbps 2.5
157 *
158 * Note that 'link-speed' is in Mbps.
159 *
160 * 'BW' is the percentage bandwidth out of full
161 * link speed which can be found with the
162 * following equation. Note that idleSlope here
163 * is the parameter from this function
164 * which is in kbps.
165 *
166 * BW = idleSlope
167 * ----------------- (E2)
168 * link-speed * 1000
169 *
170 * That said, we can come up with a generic
171 * equation to calculate the value we should set
172 * it TQAVCC register by replacing 'BW' in E1 by E2.
173 * The resulting equation is:
174 *
175 * value = link-speed * 0x7736 * idleSlope * 0.2
176 * ------------------------------------- (E3)
177 * 100 * 2.5 * link-speed * 1000
178 *
179 * 'link-speed' is present in both sides of the
180 * fraction so it is canceled out. The final
181 * equation is the following:
182 *
183 * value = idleSlope * 61036
184 * ----------------- (E4)
185 * 2500000
186 *
187 * NOTE: For i225, given the above, we can see
188 * that idleslope is represented in
189 * 40.959433 kbps units by the value at
190 * the TQAVCC register (2.5Gbps / 61036),
191 * which reduces the granularity for
192 * idleslope increments.
193 *
194 * In i225 controller, the sendSlope and loCredit
195 * parameters from CBS are not configurable
196 * by software so we don't do any
197 * 'controller configuration' in respect to
198 * these parameters.
199 */
200 cbs_value = DIV_ROUND_UP_ULL(ring->idleslope
201 * 61036ULL, 2500000);
202
203 tqavcc = rd32(IGC_TQAVCC(i));
204 tqavcc &= ~IGC_TQAVCC_IDLESLOPE_MASK;
205 tqavcc |= cbs_value | IGC_TQAVCC_KEEP_CREDITS;
206 wr32(IGC_TQAVCC(i), tqavcc);
207
208 wr32(IGC_TQAVHC(i),
209 0x80000000 + ring->hicredit * 0x7735);
210 } else {
211 /* Disable any CBS for the queue */
212 txqctl &= ~(IGC_TXQCTL_QAV_SEL_MASK);
213
214 /* Set idleSlope to zero. */
215 tqavcc = rd32(IGC_TQAVCC(i));
216 tqavcc &= ~(IGC_TQAVCC_IDLESLOPE_MASK |
217 IGC_TQAVCC_KEEP_CREDITS);
218 wr32(IGC_TQAVCC(i), tqavcc);
219
220 /* Set hiCredit to zero. */
221 wr32(IGC_TQAVHC(i), 0);
222 }
223 skip_cbs:
224 wr32(IGC_TXQCTL(i), txqctl);
225 }
226
227 nsec = rd32(IGC_SYSTIML);
228 sec = rd32(IGC_SYSTIMH);
229
230 systim = ktime_set(sec, nsec);
231
232 if (ktime_compare(systim, base_time) > 0) {
233 s64 n;
234
235 n = div64_s64(ktime_sub_ns(systim, base_time), cycle);
236 base_time = ktime_add_ns(base_time, (n + 1) * cycle);
237 }
238
239 baset_h = div_s64_rem(base_time, NSEC_PER_SEC, &baset_l);
240
241 wr32(IGC_BASET_H, baset_h);
242 wr32(IGC_BASET_L, baset_l);
243
244 return 0;
245 }
246
Dear Muhammad, Thank you for your patch. Am 09.05.22 um 03:08 schrieb Muhammad Husaini Zulkifli: > The launchtime offset can be corrected according to sections 7.5.2.6 > and 12.3.2 of the datasheet. “can” or “should”? Sorry, what is the name of the datasheet? What problems does it fix, and how can they be reproduced? > Based on the preliminary data, this correction is roughly 1500ns. > In the future, proper measurements must be taken, and corrections must take > into account the various link speeds. Nit: Please do not wrap lines just because a sentence ends. If these are paragraphs, then please separate those by exactly one blank line. > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_regs.h | 1 + > drivers/net/ethernet/intel/igc/igc_tsn.c | 14 ++++++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h > index e197a33d93a0..928d22b1ca22 100644 > --- a/drivers/net/ethernet/intel/igc/igc_regs.h > +++ b/drivers/net/ethernet/intel/igc/igc_regs.h > @@ -231,6 +231,7 @@ > #define IGC_BASET_H 0x3318 > #define IGC_QBVCYCLET 0x331C > #define IGC_QBVCYCLET_S 0x3320 > +#define IGC_GTXOFFSET 0x3310 Sort these by the value? > > #define IGC_STQT(_n) (0x3324 + 0x4 * (_n)) > #define IGC_ENDQT(_n) (0x3334 + 0x4 * (_n)) > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c > index 0fce22de2ab8..3d4d0ab011e3 100644 > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > @@ -84,12 +84,26 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) > struct igc_hw *hw = &adapter->hw; > u32 tqavctrl, baset_l, baset_h; > u32 sec, nsec, cycle; > + u16 tx_offset; Please append the unit to the variable name. Why specify the size, and not just use `unsigned int` [1]? (`__writel()` also uses `unsigned int` in the end? > ktime_t base_time, systim; > int i; > > cycle = adapter->cycle_time; > base_time = adapter->base_time; > > + switch (adapter->link_speed) { > + /* TODO: This code use same transmit offset across all link speed as for now. */ > + case SPEED_10: > + case SPEED_100: > + case SPEED_1000: > + case SPEED_2500: > + tx_offset = 1500; > + break; > + default: > + break; > + } > + > + wr32(IGC_GTXOFFSET, tx_offset); > wr32(IGC_TSAUXC, 0); > wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_TSN); > wr32(IGC_TXPBS, IGC_TXPBSIZE_TSN); Kind regards, Paul [1]: https://notabs.org/coding/smallIntsBigPenalty.htm
Hi Paul, Sorry for the late response. > -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Tuesday, 10 May, 2022 3:04 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Gomes, Vinicius > <vinicius.gomes@intel.com> > Subject: Re: [Intel-wired-lan] [PATCH] igc: Correct the launchtime offset > > Dear Muhammad, > > > Thank you for your patch. > > Am 09.05.22 um 03:08 schrieb Muhammad Husaini Zulkifli: > > The launchtime offset can be corrected according to sections 7.5.2.6 > > and 12.3.2 of the datasheet. > > “can” or “should”? I will rephrase this statement in v2. > > Sorry, what is the name of the datasheet? > > What problems does it fix, and how can they be reproduced? This is to reduce the transmit latency between transmission schedule(launchtime) And the time that packet been transmitted to network. Software can compensate this by setting the GTXOFFSET value. It can be reproduce by using l2_tai on transmit side by having a launchtime packet in the payload and on the receiver side, I am using the timedump application to measure the latency between RX HW timestamp - payload ts > > > Based on the preliminary data, this correction is roughly 1500ns. > > In the future, proper measurements must be taken, and corrections must > > take into account the various link speeds. > > Nit: Please do not wrap lines just because a sentence ends. If these are > paragraphs, then please separate those by exactly one blank line. Noted. Will fix this > > > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > > Signed-off-by: Muhammad Husaini Zulkifli > > <muhammad.husaini.zulkifli@intel.com> > > --- > > drivers/net/ethernet/intel/igc/igc_regs.h | 1 + > > drivers/net/ethernet/intel/igc/igc_tsn.c | 14 ++++++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h > > b/drivers/net/ethernet/intel/igc/igc_regs.h > > index e197a33d93a0..928d22b1ca22 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_regs.h > > +++ b/drivers/net/ethernet/intel/igc/igc_regs.h > > @@ -231,6 +231,7 @@ > > #define IGC_BASET_H 0x3318 > > #define IGC_QBVCYCLET 0x331C > > #define IGC_QBVCYCLET_S 0x3320 > > +#define IGC_GTXOFFSET 0x3310 > > Sort these by the value? Will do thanks. > > > > > #define IGC_STQT(_n) (0x3324 + 0x4 * (_n)) > > #define IGC_ENDQT(_n) (0x3334 + 0x4 * (_n)) > > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c > > b/drivers/net/ethernet/intel/igc/igc_tsn.c > > index 0fce22de2ab8..3d4d0ab011e3 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > > @@ -84,12 +84,26 @@ static int igc_tsn_enable_offload(struct > igc_adapter *adapter) > > struct igc_hw *hw = &adapter->hw; > > u32 tqavctrl, baset_l, baset_h; > > u32 sec, nsec, cycle; > > + u16 tx_offset; > > Please append the unit to the variable name. > > Why specify the size, and not just use `unsigned int` [1]? (`__writel()` also > uses `unsigned int` in the end? Because the GTXOFFSET is only 16bit. That is the reason I just use u16 here. > > > ktime_t base_time, systim; > > int i; > > > > cycle = adapter->cycle_time; > > base_time = adapter->base_time; > > > > + switch (adapter->link_speed) { > > + /* TODO: This code use same transmit offset across all link speed as > for now. */ > > + case SPEED_10: > > + case SPEED_100: > > + case SPEED_1000: > > + case SPEED_2500: > > + tx_offset = 1500; > > + break; > > + default: > > + break; > > + } I have another fine tune value respective for each link speed. Will update in v2. > > + > > + wr32(IGC_GTXOFFSET, tx_offset); > > wr32(IGC_TSAUXC, 0); > > wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_TSN); > > wr32(IGC_TXPBS, IGC_TXPBSIZE_TSN); > > > Kind regards, > > Paul > > > [1]: https://notabs.org/coding/smallIntsBigPenalty.htm
Hi Paul, Sorry for the late response. > -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Tuesday, 10 May, 2022 3:04 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Gomes, Vinicius > <vinicius.gomes@intel.com> > Subject: Re: [Intel-wired-lan] [PATCH] igc: Correct the launchtime offset > > Dear Muhammad, > > > Thank you for your patch. > > Am 09.05.22 um 03:08 schrieb Muhammad Husaini Zulkifli: > > The launchtime offset can be corrected according to sections 7.5.2.6 > > and 12.3.2 of the datasheet. > > “can” or “should”? I will rephrase this statement in v2. > > Sorry, what is the name of the datasheet? > > What problems does it fix, and how can they be reproduced? This is to reduce the transmit latency between transmission schedule(launchtime) And the time that packet been transmitted to network. Software can compensate this by setting the GTXOFFSET value. It can be reproduce by using l2_tai on transmit side by having a launchtime packet in the payload and on the receiver side, I am using the timedump application to measure the latency between RX HW timestamp - payload ts > > > Based on the preliminary data, this correction is roughly 1500ns. > > In the future, proper measurements must be taken, and corrections must > > take into account the various link speeds. > > Nit: Please do not wrap lines just because a sentence ends. If these are > paragraphs, then please separate those by exactly one blank line. Noted. Will fix this > > > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > > Signed-off-by: Muhammad Husaini Zulkifli > > <muhammad.husaini.zulkifli@intel.com> > > --- > > drivers/net/ethernet/intel/igc/igc_regs.h | 1 + > > drivers/net/ethernet/intel/igc/igc_tsn.c | 14 ++++++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h > > b/drivers/net/ethernet/intel/igc/igc_regs.h > > index e197a33d93a0..928d22b1ca22 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_regs.h > > +++ b/drivers/net/ethernet/intel/igc/igc_regs.h > > @@ -231,6 +231,7 @@ > > #define IGC_BASET_H 0x3318 > > #define IGC_QBVCYCLET 0x331C > > #define IGC_QBVCYCLET_S 0x3320 > > +#define IGC_GTXOFFSET 0x3310 > > Sort these by the value? Will do thanks. > > > > > #define IGC_STQT(_n) (0x3324 + 0x4 * (_n)) > > #define IGC_ENDQT(_n) (0x3334 + 0x4 * (_n)) > > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c > > b/drivers/net/ethernet/intel/igc/igc_tsn.c > > index 0fce22de2ab8..3d4d0ab011e3 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > > @@ -84,12 +84,26 @@ static int igc_tsn_enable_offload(struct > igc_adapter *adapter) > > struct igc_hw *hw = &adapter->hw; > > u32 tqavctrl, baset_l, baset_h; > > u32 sec, nsec, cycle; > > + u16 tx_offset; > > Please append the unit to the variable name. > > Why specify the size, and not just use `unsigned int` [1]? (`__writel()` also > uses `unsigned int` in the end? Yeah. Will change it u32 > > > ktime_t base_time, systim; > > int i; > > > > cycle = adapter->cycle_time; > > base_time = adapter->base_time; > > > > + switch (adapter->link_speed) { > > + /* TODO: This code use same transmit offset across all link speed as > for now. */ > > + case SPEED_10: > > + case SPEED_100: > > + case SPEED_1000: > > + case SPEED_2500: > > + tx_offset = 1500; > > + break; > > + default: > > + break; > > + } I have another fine tune value respective for each link speed. Will update in v2. > > + > > + wr32(IGC_GTXOFFSET, tx_offset); > > wr32(IGC_TSAUXC, 0); > > wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_TSN); > > wr32(IGC_TXPBS, IGC_TXPBSIZE_TSN); > > > Kind regards, > > Paul > > > [1]: https://notabs.org/coding/smallIntsBigPenalty.htm
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h index e197a33d93a0..928d22b1ca22 100644 --- a/drivers/net/ethernet/intel/igc/igc_regs.h +++ b/drivers/net/ethernet/intel/igc/igc_regs.h @@ -231,6 +231,7 @@ #define IGC_BASET_H 0x3318 #define IGC_QBVCYCLET 0x331C #define IGC_QBVCYCLET_S 0x3320 +#define IGC_GTXOFFSET 0x3310 #define IGC_STQT(_n) (0x3324 + 0x4 * (_n)) #define IGC_ENDQT(_n) (0x3334 + 0x4 * (_n)) diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index 0fce22de2ab8..3d4d0ab011e3 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -84,12 +84,26 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) struct igc_hw *hw = &adapter->hw; u32 tqavctrl, baset_l, baset_h; u32 sec, nsec, cycle; + u16 tx_offset; ktime_t base_time, systim; int i; cycle = adapter->cycle_time; base_time = adapter->base_time; + switch (adapter->link_speed) { + /* TODO: This code use same transmit offset across all link speed as for now. */ + case SPEED_10: + case SPEED_100: + case SPEED_1000: + case SPEED_2500: + tx_offset = 1500; + break; + default: + break; + } + + wr32(IGC_GTXOFFSET, tx_offset); wr32(IGC_TSAUXC, 0); wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_TSN); wr32(IGC_TXPBS, IGC_TXPBSIZE_TSN);