Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <92180324-fa55-5427-839e-d555ac5a6cd7@molgen.mpg.de>
Date:   Thu, 1 Jun 2023 07:33:53 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [Intel-wired-lan] [PATCH] igb: Fix extts capture value format for
 82580/i354/i350
Content-Language: en-US
To:     Yuezhen Luan <eggcar.luan@gmail.com>
Cc:     jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        intel-wired-lan@lists.osuosl.org, linux-kernel@vger.kernel.org
References: <20230531090805.3959-1-eggcar.luan@gmail.com>
From:   Paul Menzel <pmenzel@molgen.mpg.de>
In-Reply-To: <20230531090805.3959-1-eggcar.luan@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Dear Yuezhen,


Thank you very much for your patch.

Am 31.05.23 um 11:08 schrieb Yuezhen Luan:
> 82580/i354/i350 features circle-counter-like timestamp registers
> that are different with newer i210. The EXTTS capture value in
> AUXTSMPx should be converted from raw circle counter value to
> timestamp value in resolution of 1 nanosec by the driver.

It’d be great, if you added a paragraph how to reproduce the issue.

> Signed-off-by: Yuezhen Luan <eggcar.luan@gmail.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 58872a4c2..187daa8ef 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6947,6 +6947,7 @@ static void igb_extts(struct igb_adapter *adapter, int tsintr_tt)
>   	struct e1000_hw *hw = &adapter->hw;
>   	struct ptp_clock_event event;
>   	struct timespec64 ts;
> +	unsigned long flags;

I do not see the variable *flags* being used.

>   
>   	if (pin < 0 || pin >= IGB_N_SDP)
>   		return;
> @@ -6954,9 +6955,12 @@ static void igb_extts(struct igb_adapter *adapter, int tsintr_tt)
>   	if (hw->mac.type == e1000_82580 ||
>   	    hw->mac.type == e1000_i354 ||
>   	    hw->mac.type == e1000_i350) {
> -		s64 ns = rd32(auxstmpl);
> +		u64 ns = rd32(auxstmpl);
>   
> -		ns += ((s64)(rd32(auxstmph) & 0xFF)) << 32;
> +		ns += ((u64)(rd32(auxstmph) & 0xFF)) << 32;
> +		spin_lock_irqsave(&adapter->tc, ns);
> +		ns = timecounter_cyc2time(&adapter->tc, ns);
> +		spin_unlock_irqrestore(&adapter->tc, ns);
>   		ts = ns_to_timespec64(ns);
>   	} else {
>   		ts.tv_nsec = rd32(auxstmpl);


Kind regards,

Paul