[PATCH net-next 0/7] Fixing the "Time Counter fixes and improvements"

[PATCH net-next 0/7] Fixing the "Time Counter fixes and improvements"

[Richard Cochran]

Dave,

I did not catch the missing includes in my x86 and arm testing,
because those archs somehow do include clocksource.h for the drivers
in question. Sorry.

This is how I would like to fix the header fallout. We really should
decouple the timecounter/cyclecounter code from the clocksource code
where possible.


Thanks,
Richard


Richard Cochran (7):
  timecounter: provide a macro to initialize the cyclecounter mask
    field.
  bnx2x: convert to CYCLECOUNTER_MASK macro.
  e1000e: convert to CYCLECOUNTER_MASK macro.
  igb: convert to CYCLECOUNTER_MASK macro.
  ixgbe: convert to CYCLECOUNTER_MASK macro.
  mlx4: include clocksource.h again
  microblaze: include the new timecounter header.

 arch/microblaze/kernel/timer.c                   |    1 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    2 +-
 drivers/net/ethernet/intel/e1000e/netdev.c       |    2 +-
 drivers/net/ethernet/intel/igb/igb_ptp.c         |    4 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c     |    2 +-
 drivers/net/ethernet/mellanox/mlx4/en_clock.c    |    1 +
 include/linux/timecounter.h                      |    5 ++++-
 7 files changed, 11 insertions(+), 6 deletions(-)

-- 
1.7.10.4

[PATCH net-next 1/7] timecounter: provide a macro to initialize the cyclecounter mask field.

[Richard Cochran]

There is no need for users of the timecounter/cyclecounter code to include
clocksource.h just for a single macro.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 include/linux/timecounter.h |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
index 74f4549..4382035 100644
--- a/include/linux/timecounter.h
+++ b/include/linux/timecounter.h
@@ -19,6 +19,9 @@
 
 #include <linux/types.h>
 
+/* simplify initialization of mask field */
+#define CYCLECOUNTER_MASK(bits) (cycle_t)((bits) < 64 ? ((1ULL<<(bits))-1) : -1)
+
 /**
  * struct cyclecounter - hardware abstraction for a free running counter
  *	Provides completely state-free accessors to the underlying hardware.
@@ -29,7 +32,7 @@
  * @read:		returns the current cycle value
  * @mask:		bitmask for two's complement
  *			subtraction of non 64 bit counters,
- *			see CLOCKSOURCE_MASK() helper macro
+ *			see CYCLECOUNTER_MASK() helper macro
  * @mult:		cycle to nanosecond multiplier
  * @shift:		cycle to nanosecond divisor (power of two)
  */
-- 
1.7.10.4

[PATCH net-next 2/7] bnx2x: convert to CYCLECOUNTER_MASK macro.

[Richard Cochran]

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 2c95132..0758c8b 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -14610,7 +14610,7 @@ static void bnx2x_init_cyclecounter(struct bnx2x *bp)
 {
 	memset(&bp->cyclecounter, 0, sizeof(bp->cyclecounter));
 	bp->cyclecounter.read = bnx2x_cyclecounter_read;
-	bp->cyclecounter.mask = CLOCKSOURCE_MASK(64);
+	bp->cyclecounter.mask = CYCLECOUNTER_MASK(64);
 	bp->cyclecounter.shift = 1;
 	bp->cyclecounter.mult = 1;
 }
-- 
1.7.10.4

[PATCH net-next 3/7] e1000e: convert to CYCLECOUNTER_MASK macro.

[Richard Cochran]

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index e14fd85..332a298 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4189,7 +4189,7 @@ static int e1000_sw_init(struct e1000_adapter *adapter)
 	/* Setup hardware time stamping cyclecounter */
 	if (adapter->flags & FLAG_HAS_HW_TIMESTAMP) {
 		adapter->cc.read = e1000e_cyclecounter_read;
-		adapter->cc.mask = CLOCKSOURCE_MASK(64);
+		adapter->cc.mask = CYCLECOUNTER_MASK(64);
 		adapter->cc.mult = 1;
 		/* cc.shift set in e1000e_get_base_tininca() */
 
-- 
1.7.10.4

[PATCH net-next 4/7] igb: convert to CYCLECOUNTER_MASK macro.

[Richard Cochran]

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb_ptp.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 1d27f2d..5e7a4e3 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -765,7 +765,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.settime = igb_ptp_settime_82576;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable;
 		adapter->cc.read = igb_ptp_read_82576;
-		adapter->cc.mask = CLOCKSOURCE_MASK(64);
+		adapter->cc.mask = CYCLECOUNTER_MASK(64);
 		adapter->cc.mult = 1;
 		adapter->cc.shift = IGB_82576_TSYNC_SHIFT;
 		/* Dial the nominal frequency. */
@@ -785,7 +785,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.settime = igb_ptp_settime_82576;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable;
 		adapter->cc.read = igb_ptp_read_82580;
-		adapter->cc.mask = CLOCKSOURCE_MASK(IGB_NBITS_82580);
+		adapter->cc.mask = CYCLECOUNTER_MASK(IGB_NBITS_82580);
 		adapter->cc.mult = 1;
 		adapter->cc.shift = 0;
 		/* Enable the timer functions by clearing bit 31. */
-- 
1.7.10.4

[PATCH net-next 5/7] ixgbe: convert to CYCLECOUNTER_MASK macro.

[Richard Cochran]

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index 47c29ea..79c00f5 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -793,7 +793,7 @@ void ixgbe_ptp_start_cyclecounter(struct ixgbe_adapter *adapter)
 
 	memset(&adapter->cc, 0, sizeof(adapter->cc));
 	adapter->cc.read = ixgbe_ptp_read;
-	adapter->cc.mask = CLOCKSOURCE_MASK(64);
+	adapter->cc.mask = CYCLECOUNTER_MASK(64);
 	adapter->cc.shift = shift;
 	adapter->cc.mult = 1;
 
-- 
1.7.10.4

[PATCH net-next 6/7] mlx4: include clocksource.h again

[Richard Cochran]

This driver uses the function, clocksource_khz2mult, and so it really must
include clocksource.h.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_clock.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
index e9cce4f..90b5309 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
@@ -32,6 +32,7 @@
  */
 
 #include <linux/mlx4/device.h>
+#include <linux/clocksource.h>
 
 #include "mlx4_en.h"
 
-- 
1.7.10.4

[PATCH net-next 7/7] microblaze: include the new timecounter header.

[Richard Cochran]

The timecounter/cyclecounter code has moved, so users need the new include.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 arch/microblaze/kernel/timer.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/microblaze/kernel/timer.c b/arch/microblaze/kernel/timer.c
index dd96f0e..c897745 100644
--- a/arch/microblaze/kernel/timer.c
+++ b/arch/microblaze/kernel/timer.c
@@ -17,6 +17,7 @@
 #include <linux/clockchips.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/timecounter.h>
 #include <asm/cpuinfo.h>
 
 static void __iomem *timer_baseaddr;
-- 
1.7.10.4

RE: [PATCH net-next 1/7] timecounter: provide a macro to initialize the cyclecounter mask field.

[David Laight]

> There is no need for users of the timecounter/cyclecounter code to include
> clocksource.h just for a single macro.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
>  include/linux/timecounter.h |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
> index 74f4549..4382035 100644
> --- a/include/linux/timecounter.h
> +++ b/include/linux/timecounter.h
> @@ -19,6 +19,9 @@
> 
>  #include <linux/types.h>
> 
> +/* simplify initialization of mask field */
> +#define CYCLECOUNTER_MASK(bits) (cycle_t)((bits) < 64 ? ((1ULL<<(bits))-1) : -1)

That has me chasing through the C integer promotion rules.
Better might be:
	((bits) < 64 ? (1ULL << (bits)) - 1 : (((1ULL << 63) - 1) << 1) + 1)
I actually suspect there is a standard definition somewhere?

	David 

Re: [PATCH net-next 1/7] timecounter: provide a macro to initialize the cyclecounter mask field.

[Richard Cochran]

On Mon, Jan 05, 2015 at 01:20:57PM +0000, David Laight wrote:
> > +/* simplify initialization of mask field */
> > +#define CYCLECOUNTER_MASK(bits) (cycle_t)((bits) < 64 ? ((1ULL<<(bits))-1) : -1)
> 
> That has me chasing through the C integer promotion rules.
> Better might be:
> 	((bits) < 64 ? (1ULL << (bits)) - 1 : (((1ULL << 63) - 1) << 1) + 1)
> I actually suspect there is a standard definition somewhere?

This is an exact copy of CLOCKSOURCE_MASK, and if wrong, then both are
wrong.  In any case, I can't see any issue here. Is not

	(some_int_type) -1

always equal to

	0xf...(width of type)

for all integer types, when using 2s compliment?

Thanks
Richard

RE: [PATCH net-next 1/7] timecounter: provide a macro to initialize the cyclecounter mask field.

[David Laight]

> On Mon, Jan 05, 2015 at 01:20:57PM +0000, David Laight wrote:
> > > +/* simplify initialization of mask field */
> > > +#define CYCLECOUNTER_MASK(bits) (cycle_t)((bits) < 64 ? ((1ULL<<(bits))-1) : -1)
> >
> > That has me chasing through the C integer promotion rules.
> > Better might be:
> > 	((bits) < 64 ? (1ULL << (bits)) - 1 : (((1ULL << 63) - 1) << 1) + 1)
> > I actually suspect there is a standard definition somewhere?
> 
> This is an exact copy of CLOCKSOURCE_MASK, and if wrong, then both are
> wrong.  In any case, I can't see any issue here. Is not
> 
> 	(some_int_type) -1
> 
> always equal to
> 
> 	0xf...(width of type)
> 
> for all integer types, when using 2s compliment?

As I said, it leaves me chasing through the promotion rules (which I
probably know if I actually think hard enough).

Thinking... ~0ULL would be nice and simple and correct.

	David