| Message ID | 20201124220259.3027991-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,net] enetc: Let the hardware auto-advance the taprio base-time of 0 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 29 this patch: 29 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 29 this patch: 29 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
It makes sense to me for this patch. Thanks! > -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: 2020年11月25日 6:03 > To: Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; Po Liu > <po.liu@nxp.com>; Claudiu Manoil <claudiu.manoil@nxp.com> > Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com> > Subject: [PATCH v3 net] enetc: Let the hardware auto-advance the taprio > base-time of 0 > > The tc-taprio base time indicates the beginning of the tc-taprio schedule, > which is cyclic by definition (where the length of the cycle in nanoseconds > is called the cycle time). The base time is a 64-bit PTP time in the TAI > domain. > > Logically, the base-time should be a future time. But that imposes some > restrictions to user space, which has to retrieve the current PTP time from > the NIC first, then calculate a base time that will still be larger than the > base time by the time the kernel driver programs this value into the > hardware. Actually ensuring that the programmed base time is in the > future is still a problem even if the kernel alone deals with this. > > Luckily, the enetc hardware already advances a base-time that is in the > past into a congruent time in the immediate future, according to the same > formula that can be found in the software implementation of taprio (in > taprio_get_start_time): > > /* Schedule the start time for the beginning of the next > * cycle. > */ > n = div64_s64(ktime_sub_ns(now, base), cycle); > *start = ktime_add_ns(base, (n + 1) * cycle); > > There's only one problem: the driver doesn't let the hardware do that. > It interferes with the base-time passed from user space, by special-casing > the situation when the base-time is zero, and replaces that with the > current PTP time. This changes the intended effective base-time of the > schedule, which will in the end have a different phase offset than if the > base-time of 0.000000000 was to be advanced by an integer multiple of > the cycle-time. > > Fixes: 34c6adf1977b ("enetc: Configure the Time-Aware Scheduler via tc- > taprio offload") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > Changes in v3: > - Removed an obsolete phrase from commit message. > > Changes in v2: > - Now letting the hardware completely deal with advancing base times in > the past. > > drivers/net/ethernet/freescale/enetc/enetc_qos.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c > b/drivers/net/ethernet/freescale/enetc/enetc_qos.c > index aeb21dc48099..a9aee219fb58 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c > @@ -92,18 +92,8 @@ static int enetc_setup_taprio(struct net_device > *ndev, > 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->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); > > -- > 2.25.1
On Wed, 25 Nov 2020 02:28:27 +0000 Po Liu wrote: > > The tc-taprio base time indicates the beginning of the tc-taprio schedule, > > which is cyclic by definition (where the length of the cycle in nanoseconds > > is called the cycle time). The base time is a 64-bit PTP time in the TAI > > domain. > > > > Logically, the base-time should be a future time. But that imposes some > > restrictions to user space, which has to retrieve the current PTP time from > > the NIC first, then calculate a base time that will still be larger than the > > base time by the time the kernel driver programs this value into the > > hardware. Actually ensuring that the programmed base time is in the > > future is still a problem even if the kernel alone deals with this. > > > > Luckily, the enetc hardware already advances a base-time that is in the > > past into a congruent time in the immediate future, according to the same > > formula that can be found in the software implementation of taprio (in > > taprio_get_start_time): > > > > /* Schedule the start time for the beginning of the next > > * cycle. > > */ > > n = div64_s64(ktime_sub_ns(now, base), cycle); > > *start = ktime_add_ns(base, (n + 1) * cycle); > > > > There's only one problem: the driver doesn't let the hardware do that. > > It interferes with the base-time passed from user space, by special-casing > > the situation when the base-time is zero, and replaces that with the > > current PTP time. This changes the intended effective base-time of the > > schedule, which will in the end have a different phase offset than if the > > base-time of 0.000000000 was to be advanced by an integer multiple of > > the cycle-time. > > > > Fixes: 34c6adf1977b ("enetc: Configure the Time-Aware Scheduler via tc- > > taprio offload") > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > It makes sense to me for this patch. Thanks! Applied, thanks!
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c index aeb21dc48099..a9aee219fb58 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c @@ -92,18 +92,8 @@ static int enetc_setup_taprio(struct net_device *ndev, 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->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);
The tc-taprio base time indicates the beginning of the tc-taprio schedule, which is cyclic by definition (where the length of the cycle in nanoseconds is called the cycle time). The base time is a 64-bit PTP time in the TAI domain. Logically, the base-time should be a future time. But that imposes some restrictions to user space, which has to retrieve the current PTP time from the NIC first, then calculate a base time that will still be larger than the base time by the time the kernel driver programs this value into the hardware. Actually ensuring that the programmed base time is in the future is still a problem even if the kernel alone deals with this. Luckily, the enetc hardware already advances a base-time that is in the past into a congruent time in the immediate future, according to the same formula that can be found in the software implementation of taprio (in taprio_get_start_time): /* Schedule the start time for the beginning of the next * cycle. */ n = div64_s64(ktime_sub_ns(now, base), cycle); *start = ktime_add_ns(base, (n + 1) * cycle); There's only one problem: the driver doesn't let the hardware do that. It interferes with the base-time passed from user space, by special-casing the situation when the base-time is zero, and replaces that with the current PTP time. This changes the intended effective base-time of the schedule, which will in the end have a different phase offset than if the base-time of 0.000000000 was to be advanced by an integer multiple of the cycle-time. Fixes: 34c6adf1977b ("enetc: Configure the Time-Aware Scheduler via tc-taprio offload") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- Changes in v3: - Removed an obsolete phrase from commit message. Changes in v2: - Now letting the hardware completely deal with advancing base times in the past. drivers/net/ethernet/freescale/enetc/enetc_qos.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)