Re: [PATCH net-next 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method

[Jesse Brandeburg <jesse.brandeburg@intel.com>]

On Mon, 2011-12-12 at 19:00 -0800, Richard Cochran wrote:
> This commit removes the legacy timecompare code from the igb driver and
> offers a tunable PHC instead.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>

Richard, first, thanks for this work, I have some feedback and request
you make a V2.

> -       /*
> -        * The timestamp latches on lowest register read. For the 82580
> -        * the lowest register is SYSTIMR instead of SYSTIML.  However we never
> -        * adjusted TIMINCA so SYSTIMR will just read as all 0s so ignore it.
> -        */

Please keep this comment in your new igb_82580_systim_read, it explains
a bit of *why* we are doing something.  There were a lot of explanatory
comments that you removed, please audit the "-" lines of your patch and
add back the comments that are appropriate in your new code. 

> -       if (hw->mac.type >= e1000_82580) {
> -               stamp = rd32(E1000_SYSTIMR) >> 8;
> -               shift = IGB_82580_TSYNC_SHIFT;
> -       }
> -
> -       stamp |= (u64)rd32(E1000_SYSTIML) << shift;
> -       stamp |= (u64)rd32(E1000_SYSTIMH) << (shift + 32);
> -       return stamp;
> -}
> -
>  /**
>   * igb_get_hw_dev - return device
>   * used by hardware layer to print debugging information
> @@ -2080,7 +2052,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
> 
>  #endif
>         /* do hw tstamp init after resetting */
> -       igb_init_hw_timer(adapter);
> +       igb_ptp_init(adapter);
> 
>         dev_info(&pdev->dev, "Intel(R) Gigabit Ethernet Network Connection\n");
>         /* print bus type/speed/width info */
> @@ -2150,6 +2122,8 @@ static void __devexit igb_remove(struct pci_dev *pdev)
>         struct igb_adapter *adapter = netdev_priv(netdev);
>         struct e1000_hw *hw = &adapter->hw;
> 
> +       igb_ptp_remove(adapter);
> +
>         /*
>          * The watchdog timer may be rescheduled, so explicitly
>          * disable watchdog from being rescheduled.
> @@ -2269,112 +2243,6 @@ out:
>  }
> 
>  /**
> - * igb_init_hw_timer - Initialize hardware timer used with IEEE 1588 timestamp
> - * @adapter: board private structure to initialize
> - *
> - * igb_init_hw_timer initializes the function pointer and values for the hw
> - * timer found in hardware.
> - **/
> -static void igb_init_hw_timer(struct igb_adapter *adapter)
> -{
> -       struct e1000_hw *hw = &adapter->hw;
> -
> -       switch (hw->mac.type) {
> -       case e1000_i350:
> -       case e1000_82580:
> -               memset(&adapter->cycles, 0, sizeof(adapter->cycles));
> -               adapter->cycles.read = igb_read_clock;
> -               adapter->cycles.mask = CLOCKSOURCE_MASK(64);
> -               adapter->cycles.mult = 1;
> -               /*
> -                * The 82580 timesync updates the system timer every 8ns by 8ns
> -                * and the value cannot be shifted.  Instead we need to shift
> -                * the registers to generate a 64bit timer value.  As a result
> -                * SYSTIMR/L/H, TXSTMPL/H, RXSTMPL/H all have to be shifted by
> -                * 24 in order to generate a larger value for synchronization.
> -                */
> -               adapter->cycles.shift = IGB_82580_TSYNC_SHIFT;
> -               /* disable system timer temporarily by setting bit 31 */
> -               wr32(E1000_TSAUXC, 0x80000000);
> -               wrfl();
> -
> -               /* Set registers so that rollover occurs soon to test this. */
> -               wr32(E1000_SYSTIMR, 0x00000000);
> -               wr32(E1000_SYSTIML, 0x80000000);
> -               wr32(E1000_SYSTIMH, 0x000000FF);
> -               wrfl();
> -
> -               /* enable system timer by clearing bit 31 */
> -               wr32(E1000_TSAUXC, 0x0);
> -               wrfl();
> -
> -               timecounter_init(&adapter->clock,
> -                                &adapter->cycles,
> -                                ktime_to_ns(ktime_get_real()));
> -               /*
> -                * Synchronize our NIC clock against system wall clock. NIC
> -                * time stamp reading requires ~3us per sample, each sample
> -                * was pretty stable even under load => only require 10
> -                * samples for each offset comparison.
> -                */
> -               memset(&adapter->compare, 0, sizeof(adapter->compare));
> -               adapter->compare.source = &adapter->clock;
> -               adapter->compare.target = ktime_get_real;
> -               adapter->compare.num_samples = 10;
> -               timecompare_update(&adapter->compare, 0);
> -               break;
> -       case e1000_82576:
> -               /*
> -                * Initialize hardware timer: we keep it running just in case
> -                * that some program needs it later on.
> -                */
> -               memset(&adapter->cycles, 0, sizeof(adapter->cycles));
> -               adapter->cycles.read = igb_read_clock;
> -               adapter->cycles.mask = CLOCKSOURCE_MASK(64);
> -               adapter->cycles.mult = 1;
> -               /**
> -                * Scale the NIC clock cycle by a large factor so that
> -                * relatively small clock corrections can be added or
> -                * subtracted at each clock tick. The drawbacks of a large
> -                * factor are a) that the clock register overflows more quickly
> -                * (not such a big deal) and b) that the increment per tick has
> -                * to fit into 24 bits.  As a result we need to use a shift of
> -                * 19 so we can fit a value of 16 into the TIMINCA register.
> -                */
> -               adapter->cycles.shift = IGB_82576_TSYNC_SHIFT;
> -               wr32(E1000_TIMINCA,
> -                               (1 << E1000_TIMINCA_16NS_SHIFT) |
> -                               (16 << IGB_82576_TSYNC_SHIFT));
> -
> -               /* Set registers so that rollover occurs soon to test this. */
> -               wr32(E1000_SYSTIML, 0x00000000);
> -               wr32(E1000_SYSTIMH, 0xFF800000);
> -               wrfl();
> -
> -               timecounter_init(&adapter->clock,
> -                                &adapter->cycles,
> -                                ktime_to_ns(ktime_get_real()));
> -               /*
> -                * Synchronize our NIC clock against system wall clock. NIC
> -                * time stamp reading requires ~3us per sample, each sample
> -                * was pretty stable even under load => only require 10
> -                * samples for each offset comparison.
> -                */
> -               memset(&adapter->compare, 0, sizeof(adapter->compare));
> -               adapter->compare.source = &adapter->clock;
> -               adapter->compare.target = ktime_get_real;
> -               adapter->compare.num_samples = 10;
> -               timecompare_update(&adapter->compare, 0);
> -               break;
> -       case e1000_82575:
> -               /* 82575 does not support timesync */
> -       default:
> -               break;
> -       }
> -
> -}
> -
> -/**
>   * igb_sw_init - Initialize general software structures (struct igb_adapter)
>   * @adapter: board private structure to initialize
>   *
> @@ -5628,35 +5496,6 @@ static int igb_poll(struct napi_struct *napi, int budget)
>  }
> 
>  /**
> - * igb_systim_to_hwtstamp - convert system time value to hw timestamp
> - * @adapter: board private structure
> - * @shhwtstamps: timestamp structure to update
> - * @regval: unsigned 64bit system time value.
> - *
> - * We need to convert the system time value stored in the RX/TXSTMP registers
> - * into a hwtstamp which can be used by the upper level timestamping functions
> - */
> -static void igb_systim_to_hwtstamp(struct igb_adapter *adapter,
> -                                   struct skb_shared_hwtstamps *shhwtstamps,
> -                                   u64 regval)
> -{
> -       u64 ns;
> -
> -       /*
> -        * The 82580 starts with 1ns at bit 0 in RX/TXSTMPL, shift this up to
> -        * 24 to match clock shift we setup earlier.
> -        */
> -       if (adapter->hw.mac.type >= e1000_82580)
> -               regval <<= IGB_82580_TSYNC_SHIFT;
> -
> -       ns = timecounter_cyc2time(&adapter->clock, regval);
> -       timecompare_update(&adapter->compare, ns);
> -       memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps));
> -       shhwtstamps->hwtstamp = ns_to_ktime(ns);
> -       shhwtstamps->syststamp = timecompare_transform(&adapter->compare, ns);
> -}
> -
> -/**
>   * igb_tx_hwtstamp - utility function which checks for TX time stamp
>   * @q_vector: pointer to q_vector containing needed info
>   * @buffer: pointer to igb_tx_buffer structure
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 7a9f6bc..7a62980 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -406,3 +406,45 @@ void igb_ptp_remove(struct igb_adapter *adapter)
>                          adapter->netdev->name);
>         }
>  }
> +
> +/**
> + * igb_systim_to_hwtstamp - convert system time value to hw timestamp
> + * @adapter: board private structure
> + * @shhwtstamps: timestamp structure to update

cut and paste typo? should match the arguments below.

> + * @systim: unsigned 64bit system time value.
> + *
> + * We need to convert the system time value stored in the RX/TXSTMP registers
> + * into a hwtstamp which can be used by the upper level timestamping functions
> + */
> +void igb_systim_to_hwtstamp(struct igb_adapter *adapter,
> +                           struct skb_shared_hwtstamps *hwtstamps,
> +                           u64 systim)
> +{
> +       u64 ns;
> +       unsigned long flags;
> +       unsigned int shift;
> +       int msb_set;
> +
> +       switch (adapter->hw.mac.type) {
> +       case e1000_i350:
> +       case e1000_82580:
> +               shift = OFL_SHIFT_82580;
> +               msb_set = (systim >> 32) & SYSTIMH_MSB_82580;
> +               break;
> +       case e1000_82576:
> +               shift = OFL_SHIFT_82576;
> +               msb_set = (systim >> 32) & SYSTIMH_MSB_82576;
> +               break;
> +       default:
> +               return;
> +       }
> +
> +       spin_lock_irqsave(&adapter->tmreg_lock, flags);

based on the discussion on the list with eric dumazet it should have a
comment here why the spinlock is needed and about how it is expected to
impact performance.
 
> +
> +       ns = igb_overflow_get(adapter, systim, msb_set, shift);
> +
> +       spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
> +
> +       memset(hwtstamps, 0, sizeof(*hwtstamps));
> +       hwtstamps->hwtstamp = ns_to_ktime(ns);
> +}