[Intel-wired-lan] [PATCH net-next v2] igb: refactor igb_ptp_adjfine_82580 to use diff_by_scaled_ppm

[Intel-wired-lan] [PATCH net-next v2] igb: refactor igb_ptp_adjfine_82580 to use diff_by_scaled_ppm

[Andrii Staikov]

Driver's .adjfine interface functions use adjust_by_scaled_ppm and
diff_by_scaled_ppm introduced in commit 1060707e3809
("ptp: introduce helpers to adjust by scaled parts per million")
to calculate the required adjustment in a concise manner,
but not igb_ptp_adjfine_82580.
Fix it by introducing IGB_82580_BASE_PERIODand changing function logic
to use diff_by_scaled_ppm.

Fixes: 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million")
Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
---
v2: Changing target to net-queue and reflowing commit message
to fit 75 characters per line.
---
 drivers/net/ethernet/intel/igb/igb_ptp.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 6f471b91f562..405886ee5261 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -67,6 +67,7 @@
 #define INCVALUE_82576_MASK		GENMASK(E1000_TIMINCA_16NS_SHIFT - 1, 0)
 #define INCVALUE_82576			(16u << IGB_82576_TSYNC_SHIFT)
 #define IGB_NBITS_82580			40
+#define IGB_82580_BASE_PERIOD		0x800000000
 
 static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter);
 static void igb_ptp_sdp_init(struct igb_adapter *adapter);
@@ -209,17 +210,11 @@ static int igb_ptp_adjfine_82580(struct ptp_clock_info *ptp, long scaled_ppm)
 	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
 					       ptp_caps);
 	struct e1000_hw *hw = &igb->hw;
-	int neg_adj = 0;
+	bool neg_adj;
 	u64 rate;
 	u32 inca;
 
-	if (scaled_ppm < 0) {
-		neg_adj = 1;
-		scaled_ppm = -scaled_ppm;
-	}
-	rate = scaled_ppm;
-	rate <<= 13;
-	rate = div_u64(rate, 15625);
+	neg_adj = diff_by_scaled_ppm(IGB_82580_BASE_PERIOD, scaled_ppm, &rate);
 
 	inca = rate & INCVALUE_MASK;
 	if (neg_adj)
-- 
2.25.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igb: refactor igb_ptp_adjfine_82580 to use diff_by_scaled_ppm

[Jacob Keller]

On 3/8/2023 2:36 AM, Andrii Staikov wrote:
> Driver's .adjfine interface functions use adjust_by_scaled_ppm and
> diff_by_scaled_ppm introduced in commit 1060707e3809
> ("ptp: introduce helpers to adjust by scaled parts per million")
> to calculate the required adjustment in a concise manner,
> but not igb_ptp_adjfine_82580.
> Fix it by introducing IGB_82580_BASE_PERIODand changing function logic
> to use diff_by_scaled_ppm.
> 
> Fixes: 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million")
> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>

Thanks for this. I missed this function when doing the refactor above.
This change looks correct to me.

I'm not sure this would qualify as a bug fix sent to net, but:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
> v2: Changing target to net-queue and reflowing commit message
> to fit 75 characters per line.

The subject still says this targets net-next, but I think that's fine.

> ---
>  drivers/net/ethernet/intel/igb/igb_ptp.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 6f471b91f562..405886ee5261 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -67,6 +67,7 @@
>  #define INCVALUE_82576_MASK		GENMASK(E1000_TIMINCA_16NS_SHIFT - 1, 0)
>  #define INCVALUE_82576			(16u << IGB_82576_TSYNC_SHIFT)
>  #define IGB_NBITS_82580			40
> +#define IGB_82580_BASE_PERIOD		0x800000000
>  
>  static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter);
>  static void igb_ptp_sdp_init(struct igb_adapter *adapter);
> @@ -209,17 +210,11 @@ static int igb_ptp_adjfine_82580(struct ptp_clock_info *ptp, long scaled_ppm)
>  	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
>  					       ptp_caps);
>  	struct e1000_hw *hw = &igb->hw;
> -	int neg_adj = 0;
> +	bool neg_adj;
>  	u64 rate;
>  	u32 inca;
>  
> -	if (scaled_ppm < 0) {
> -		neg_adj = 1;
> -		scaled_ppm = -scaled_ppm;
> -	}
> -	rate = scaled_ppm;
> -	rate <<= 13;
> -	rate = div_u64(rate, 15625);
> +	neg_adj = diff_by_scaled_ppm(IGB_82580_BASE_PERIOD, scaled_ppm, &rate);
>  
>  	inca = rate & INCVALUE_MASK;
>  	if (neg_adj)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] igb: refactor igb_ptp_adjfine_82580 to use diff_by_scaled_ppm

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Keller, Jacob E
> Sent: Thursday, March 9, 2023 3:22 AM
> To: intel-wired-lan@osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net-next v2] igb: refactor igb_ptp_adjfine_82580 to use diff_by_scaled_ppm
>
>
>
> On 3/8/2023 2:36 AM, Andrii Staikov wrote:
> Driver's .adjfine interface functions use adjust_by_scaled_ppm and 
> diff_by_scaled_ppm introduced in commit 1060707e3809
> ("ptp: introduce helpers to adjust by scaled parts per million") to 
> calculate the required adjustment in a concise manner, but not 
> igb_ptp_adjfine_82580.
> Fix it by introducing IGB_82580_BASE_PERIODand changing function logic 
> to use diff_by_scaled_ppm.
> 
> Fixes: 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts 
> per million")
> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
>
> Thanks for this. I missed this function when doing the refactor above.
> This change looks correct to me.
>
> I'm not sure this would qualify as a bug fix sent to net, but:
>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>
> ---
> v2: Changing target to net-queue and reflowing commit message to fit 
> 75 characters per line.
>
> The subject still says this targets net-next, but I think that's fine.
>
> ---
>  drivers/net/ethernet/intel/igb/igb_ptp.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>  

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan