diff mbox series

[v3,net] enetc: Let the hardware auto-advance the taprio base-time of 0

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

Checks

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

Commit Message

Vladimir Oltean Nov. 24, 2020, 10:02 p.m. UTC
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(-)

Comments

Po Liu Nov. 25, 2020, 2:28 a.m. UTC | #1
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
Jakub Kicinski Nov. 25, 2020, 8:38 p.m. UTC | #2
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 mbox series

Patch

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);