chiptod: Keep boot timestamps contiguous

Message ID 20171205031919.26644-1-oohall@gmail.com
State Accepted
Headers show
Series
  • chiptod: Keep boot timestamps contiguous
Related show

Commit Message

Oliver Dec. 5, 2017, 3:19 a.m.
Currently we reset the timebase value to (almost) zero when
synchronising the timebase of each chip to the Chip TOD network which
results in this:

[   42.374813167,5] CPU: All 80 processors called in...
[    2.222791151,5] FLASH: Found system flash: Macronix MXxxL51235F id:0
[    2.222977933,5] BT: Interface initialized, IO 0x00e4

This patch modifies the chiptod_init() process to use the current
timebase value rather than resetting it to zero. This results in the
timestamps remaining contigious from the start of hostboot until
the petikernel starts. e.g.

[   70.188811484,5] CPU: All 144 processors called in...
[   72.458004252,5] FLASH: Found system flash:  id:0
[   72.458147358,5] BT: Interface initialized, IO 0x00e4

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
Tested on a Witherspoon and a P8 FSP box. Seems to work ok in both
cases.
---
 core/init.c  |  8 +++++---
 hw/chiptod.c | 15 +++++++++++----
 2 files changed, 16 insertions(+), 7 deletions(-)

Comments

Michael Neuling Dec. 5, 2017, 3:23 a.m. | #1
On Tue, 2017-12-05 at 14:19 +1100, Oliver O'Halloran wrote:
> Currently we reset the timebase value to (almost) zero when
> synchronising the timebase of each chip to the Chip TOD network which
> results in this:
> 
> [   42.374813167,5] CPU: All 80 processors called in...
> [    2.222791151,5] FLASH: Found system flash: Macronix MXxxL51235F id:0
> [    2.222977933,5] BT: Interface initialized, IO 0x00e4
> 
> This patch modifies the chiptod_init() process to use the current
> timebase value rather than resetting it to zero. This results in the
> timestamps remaining contigious from the start of hostboot until
> the petikernel starts. e.g.
> 
> [   70.188811484,5] CPU: All 144 processors called in...
> [   72.458004252,5] FLASH: Found system flash:  id:0
> [   72.458147358,5] BT: Interface initialized, IO 0x00e4
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Hell-Yeahed-by: Michael Neuling <mikey@neuling.org>

> ---
> Tested on a Witherspoon and a P8 FSP box. Seems to work ok in both
> cases.
> ---
>  core/init.c  |  8 +++++---
>  hw/chiptod.c | 15 +++++++++++----
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/core/init.c b/core/init.c
> index 89a2758128e2..d008e2dc40f2 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -966,9 +966,11 @@ void __noreturn __nomcount main_cpu_entry(const void
> *fdt)
>  	cpu_set_sreset_enable(true);
>  
>  	/*
> -	 * Synchronize time bases. Thi resets all the TB values to a small
> -	 * value (so they appear to go backward at this point), and
> synchronize
> -	 * all core timebases to the global ChipTOD network
> +	 * Synchronize time bases. Prior to chiptod_init() the timebase
> +	 * is free-running at a frequency based on the core clock rather
> +	 * than being synchronised to the ChipTOD network. This means
> +	 * that the timestamps in early boot might be a little off compared
> +	 * to wall clock time.
>  	 */
>  	chiptod_init();
>  
> diff --git a/hw/chiptod.c b/hw/chiptod.c
> index b9e47741bf21..3a0940d2b8bc 100644
> --- a/hw/chiptod.c
> +++ b/hw/chiptod.c
> @@ -120,9 +120,6 @@
>  #define LOCAL_CORE_FIR		0x0104000C
>  #define LFIR_SWITCH_COMPLETE	PPC_BIT(18)
>  
> -/* Magic TB value. One step cycle ahead of sync */
> -#define INIT_TB	0x000000000001ff0
> -
>  /* Number of iterations for the various timeouts */
>  #define TIMEOUT_LOOPS		20000000
>  
> @@ -775,6 +772,7 @@ static void chiptod_reset_tod_errors(void)
>  
>  static void chiptod_sync_master(void *data)
>  {
> +	uint64_t initial_tb_value;
>  	bool *result = data;
>  
>  	prlog(PR_DEBUG, "Master sync on CPU PIR 0x%04x...\n",
> @@ -824,8 +822,17 @@ static void chiptod_sync_master(void *data)
>  		goto error;
>  	}
>  
> +	/*
> +	 * Load the master's current timebase value into the Chip TOD
> +	 * network. This is so we have sane timestamps across the whole
> +	 * IPL process. The Chip TOD documentation says that the loaded
> +	 * value needs to be one STEP before a SYNC. In other words,
> +	 * set the low bits to 0x1ff0.
> +	 */
> +	initial_tb_value = (mftb() & ~0x1fff) | 0x1ff0;
> +
>  	/* Chip TOD load initial value */
> -	if (xscom_writeme(TOD_CHIPTOD_LOAD_TB, INIT_TB)) {
> +	if (xscom_writeme(TOD_CHIPTOD_LOAD_TB, initial_tb_value)) {
>  		prerror("XSCOM error setting init TB\n");
>  		goto error;
>  	}
Vaidyanathan Srinivasan Dec. 5, 2017, 4:40 a.m. | #2
* Michael Neuling <mikey@neuling.org> [2017-12-05 14:23:13]:

> On Tue, 2017-12-05 at 14:19 +1100, Oliver O'Halloran wrote:
> > Currently we reset the timebase value to (almost) zero when
> > synchronising the timebase of each chip to the Chip TOD network which
> > results in this:
> > 
> > [   42.374813167,5] CPU: All 80 processors called in...
> > [    2.222791151,5] FLASH: Found system flash: Macronix MXxxL51235F id:0
> > [    2.222977933,5] BT: Interface initialized, IO 0x00e4
> > 
> > This patch modifies the chiptod_init() process to use the current
> > timebase value rather than resetting it to zero. This results in the
> > timestamps remaining contigious from the start of hostboot until
> > the petikernel starts. e.g.
> > 
> > [   70.188811484,5] CPU: All 144 processors called in...
> > [   72.458004252,5] FLASH: Found system flash:  id:0
> > [   72.458147358,5] BT: Interface initialized, IO 0x00e4
> > 
> > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> 
> Hell-Yeahed-by: Michael Neuling <mikey@neuling.org>

Thanks for the patch.  Good usability improvement for people looking
at the console.

Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>

> 
> > ---
> > Tested on a Witherspoon and a P8 FSP box. Seems to work ok in both
> > cases.
> > ---
> >  core/init.c  |  8 +++++---
> >  hw/chiptod.c | 15 +++++++++++----
> >  2 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/core/init.c b/core/init.c
> > index 89a2758128e2..d008e2dc40f2 100644
> > --- a/core/init.c
> > +++ b/core/init.c
> > @@ -966,9 +966,11 @@ void __noreturn __nomcount main_cpu_entry(const void
> > *fdt)
> >  	cpu_set_sreset_enable(true);
> >  
> >  	/*
> > -	 * Synchronize time bases. Thi resets all the TB values to a small
> > -	 * value (so they appear to go backward at this point), and
> > synchronize
> > -	 * all core timebases to the global ChipTOD network
> > +	 * Synchronize time bases. Prior to chiptod_init() the timebase
> > +	 * is free-running at a frequency based on the core clock rather
> > +	 * than being synchronised to the ChipTOD network. This means
> > +	 * that the timestamps in early boot might be a little off compared
> > +	 * to wall clock time.
> >  	 */
> >  	chiptod_init();
> >  
> > diff --git a/hw/chiptod.c b/hw/chiptod.c
> > index b9e47741bf21..3a0940d2b8bc 100644
> > --- a/hw/chiptod.c
> > +++ b/hw/chiptod.c
> > @@ -120,9 +120,6 @@
> >  #define LOCAL_CORE_FIR		0x0104000C
> >  #define LFIR_SWITCH_COMPLETE	PPC_BIT(18)
> >  
> > -/* Magic TB value. One step cycle ahead of sync */
> > -#define INIT_TB	0x000000000001ff0
> > -
> >  /* Number of iterations for the various timeouts */
> >  #define TIMEOUT_LOOPS		20000000
> >  
> > @@ -775,6 +772,7 @@ static void chiptod_reset_tod_errors(void)
> >  
> >  static void chiptod_sync_master(void *data)
> >  {
> > +	uint64_t initial_tb_value;
> >  	bool *result = data;
> >  
> >  	prlog(PR_DEBUG, "Master sync on CPU PIR 0x%04x...\n",
> > @@ -824,8 +822,17 @@ static void chiptod_sync_master(void *data)
> >  		goto error;
> >  	}
> >  
> > +	/*
> > +	 * Load the master's current timebase value into the Chip TOD
> > +	 * network. This is so we have sane timestamps across the whole
> > +	 * IPL process. The Chip TOD documentation says that the loaded
> > +	 * value needs to be one STEP before a SYNC. In other words,
> > +	 * set the low bits to 0x1ff0.
> > +	 */
> > +	initial_tb_value = (mftb() & ~0x1fff) | 0x1ff0;

Good idea to sync all TB based on current value of one of them rather
than starting from zero.

> > +
> >  	/* Chip TOD load initial value */
> > -	if (xscom_writeme(TOD_CHIPTOD_LOAD_TB, INIT_TB)) {
> > +	if (xscom_writeme(TOD_CHIPTOD_LOAD_TB, initial_tb_value)) {
> >  		prerror("XSCOM error setting init TB\n");
> >  		goto error;
> >  	}

--Vaidy
Suraj Jitindar Singh Dec. 5, 2017, 6:20 a.m. | #3
On Tue, 2017-12-05 at 14:19 +1100, Oliver O'Halloran wrote:
> Currently we reset the timebase value to (almost) zero when
> synchronising the timebase of each chip to the Chip TOD network which
> results in this:
> 
> [   42.374813167,5] CPU: All 80 processors called in...
> [    2.222791151,5] FLASH: Found system flash: Macronix MXxxL51235F
> id:0
> [    2.222977933,5] BT: Interface initialized, IO 0x00e4
> 
> This patch modifies the chiptod_init() process to use the current
> timebase value rather than resetting it to zero. This results in the
> timestamps remaining contigious from the start of hostboot until
> the petikernel starts. e.g.

Nak - what is a petikernel?

> 
> [   70.188811484,5] CPU: All 144 processors called in...
> [   72.458004252,5] FLASH: Found system flash:  id:0
> [   72.458147358,5] BT: Interface initialized, IO 0x00e4
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> Tested on a Witherspoon and a P8 FSP box. Seems to work ok in both
> cases.
> ---
>  core/init.c  |  8 +++++---
>  hw/chiptod.c | 15 +++++++++++----
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/core/init.c b/core/init.c
> index 89a2758128e2..d008e2dc40f2 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -966,9 +966,11 @@ void __noreturn __nomcount main_cpu_entry(const
> void *fdt)
>  	cpu_set_sreset_enable(true);
>  
>  	/*
> -	 * Synchronize time bases. Thi resets all the TB values to a
> small
> -	 * value (so they appear to go backward at this point), and
> synchronize
> -	 * all core timebases to the global ChipTOD network
> +	 * Synchronize time bases. Prior to chiptod_init() the
> timebase
> +	 * is free-running at a frequency based on the core clock
> rather
> +	 * than being synchronised to the ChipTOD network. This
> means
> +	 * that the timestamps in early boot might be a little off
> compared
> +	 * to wall clock time.
>  	 */
>  	chiptod_init();
>  
> diff --git a/hw/chiptod.c b/hw/chiptod.c
> index b9e47741bf21..3a0940d2b8bc 100644
> --- a/hw/chiptod.c
> +++ b/hw/chiptod.c
> @@ -120,9 +120,6 @@
>  #define LOCAL_CORE_FIR		0x0104000C
>  #define LFIR_SWITCH_COMPLETE	PPC_BIT(18)
>  
> -/* Magic TB value. One step cycle ahead of sync */
> -#define INIT_TB	0x000000000001ff0
> -
>  /* Number of iterations for the various timeouts */
>  #define TIMEOUT_LOOPS		20000000
>  
> @@ -775,6 +772,7 @@ static void chiptod_reset_tod_errors(void)
>  
>  static void chiptod_sync_master(void *data)
>  {
> +	uint64_t initial_tb_value;
>  	bool *result = data;
>  
>  	prlog(PR_DEBUG, "Master sync on CPU PIR 0x%04x...\n",
> @@ -824,8 +822,17 @@ static void chiptod_sync_master(void *data)
>  		goto error;
>  	}
>  
> +	/*
> +	 * Load the master's current timebase value into the Chip
> TOD
> +	 * network. This is so we have sane timestamps across the
> whole
> +	 * IPL process. The Chip TOD documentation says that the
> loaded
> +	 * value needs to be one STEP before a SYNC. In other words,
> +	 * set the low bits to 0x1ff0.
> +	 */
> +	initial_tb_value = (mftb() & ~0x1fff) | 0x1ff0;
> +
>  	/* Chip TOD load initial value */
> -	if (xscom_writeme(TOD_CHIPTOD_LOAD_TB, INIT_TB)) {
> +	if (xscom_writeme(TOD_CHIPTOD_LOAD_TB, initial_tb_value)) {
>  		prerror("XSCOM error setting init TB\n");
>  		goto error;
>  	}
Stewart Smith Dec. 13, 2017, 3:11 a.m. | #4
Oliver O'Halloran <oohall@gmail.com> writes:
> Currently we reset the timebase value to (almost) zero when
> synchronising the timebase of each chip to the Chip TOD network which
> results in this:
>
> [   42.374813167,5] CPU: All 80 processors called in...
> [    2.222791151,5] FLASH: Found system flash: Macronix MXxxL51235F id:0
> [    2.222977933,5] BT: Interface initialized, IO 0x00e4
>
> This patch modifies the chiptod_init() process to use the current
> timebase value rather than resetting it to zero. This results in the
> timestamps remaining contigious from the start of hostboot until
> the petikernel starts. e.g.
>
> [   70.188811484,5] CPU: All 144 processors called in...
> [   72.458004252,5] FLASH: Found system flash:  id:0
> [   72.458147358,5] BT: Interface initialized, IO 0x00e4
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> Tested on a Witherspoon and a P8 FSP box. Seems to work ok in both
> cases.
> ---
>  core/init.c  |  8 +++++---
>  hw/chiptod.c | 15 +++++++++++----
>  2 files changed, 16 insertions(+), 7 deletions(-)

Merged to master as of 5bb9d6f1e7dc0e38beb1db8f9f032a926310beac

Now I'm sure to be confused :)

Patch

diff --git a/core/init.c b/core/init.c
index 89a2758128e2..d008e2dc40f2 100644
--- a/core/init.c
+++ b/core/init.c
@@ -966,9 +966,11 @@  void __noreturn __nomcount main_cpu_entry(const void *fdt)
 	cpu_set_sreset_enable(true);
 
 	/*
-	 * Synchronize time bases. Thi resets all the TB values to a small
-	 * value (so they appear to go backward at this point), and synchronize
-	 * all core timebases to the global ChipTOD network
+	 * Synchronize time bases. Prior to chiptod_init() the timebase
+	 * is free-running at a frequency based on the core clock rather
+	 * than being synchronised to the ChipTOD network. This means
+	 * that the timestamps in early boot might be a little off compared
+	 * to wall clock time.
 	 */
 	chiptod_init();
 
diff --git a/hw/chiptod.c b/hw/chiptod.c
index b9e47741bf21..3a0940d2b8bc 100644
--- a/hw/chiptod.c
+++ b/hw/chiptod.c
@@ -120,9 +120,6 @@ 
 #define LOCAL_CORE_FIR		0x0104000C
 #define LFIR_SWITCH_COMPLETE	PPC_BIT(18)
 
-/* Magic TB value. One step cycle ahead of sync */
-#define INIT_TB	0x000000000001ff0
-
 /* Number of iterations for the various timeouts */
 #define TIMEOUT_LOOPS		20000000
 
@@ -775,6 +772,7 @@  static void chiptod_reset_tod_errors(void)
 
 static void chiptod_sync_master(void *data)
 {
+	uint64_t initial_tb_value;
 	bool *result = data;
 
 	prlog(PR_DEBUG, "Master sync on CPU PIR 0x%04x...\n",
@@ -824,8 +822,17 @@  static void chiptod_sync_master(void *data)
 		goto error;
 	}
 
+	/*
+	 * Load the master's current timebase value into the Chip TOD
+	 * network. This is so we have sane timestamps across the whole
+	 * IPL process. The Chip TOD documentation says that the loaded
+	 * value needs to be one STEP before a SYNC. In other words,
+	 * set the low bits to 0x1ff0.
+	 */
+	initial_tb_value = (mftb() & ~0x1fff) | 0x1ff0;
+
 	/* Chip TOD load initial value */
-	if (xscom_writeme(TOD_CHIPTOD_LOAD_TB, INIT_TB)) {
+	if (xscom_writeme(TOD_CHIPTOD_LOAD_TB, initial_tb_value)) {
 		prerror("XSCOM error setting init TB\n");
 		goto error;
 	}