[PATCH V2] cxl: Set up and enable PSL Timebase

[Daniel Axtens]

Hi Philippe,

I was trying to test Timebase Sync today and realised that it hasn't
been applied to mpe's next tree, and that it no longer applies cleanly.

Would you be able to respin it please?

I also have some minor comments inline.

On Wed, 2015-06-10 at 16:01 +0200, Philippe Bergheaud wrote:
> This patch configures the PSL Timebase function and enables it,
> after the CAPP has been initialized by OPAL.
> 
This changelog should go after a line like this:
---
> V2:
>  - Clear CXL_PSL_ErrIVTE_tberror bit
>  - Define the sync count unit
>  - Wait 1ms before each test
>  - Use negative error code
>  - Do not ignore errors
>  - Except if timebase is not supported by OPAL
>  - Be silent on success
> 
> Signed-off-by: Philippe Bergheaud <felix at linux.vnet.ibm.com>
> ---
>  drivers/misc/cxl/cxl.h |    5 ++++
>  drivers/misc/cxl/pci.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 61 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index a1cee47..38a7cf9 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -82,8 +82,10 @@ static const cxl_p1_reg_t CXL_PSL_AFUSEL  = {0x00B0};
>  /* 0x00C0:7EFF Implementation dependent area */
>  static const cxl_p1_reg_t CXL_PSL_FIR1      = {0x0100};
>  static const cxl_p1_reg_t CXL_PSL_FIR2      = {0x0108};
> +static const cxl_p1_reg_t CXL_PSL_Timebase  = {0x0110};
>  static const cxl_p1_reg_t CXL_PSL_VERSION   = {0x0118};
>  static const cxl_p1_reg_t CXL_PSL_RESLCKTO  = {0x0128};
> +static const cxl_p1_reg_t CXL_PSL_TB_CTLSTAT = {0x0140};
>  static const cxl_p1_reg_t CXL_PSL_FIR_CNTL  = {0x0148};
>  static const cxl_p1_reg_t CXL_PSL_DSNDCTL   = {0x0150};
>  static const cxl_p1_reg_t CXL_PSL_SNWRALLOC = {0x0158};
> @@ -151,6 +153,9 @@ static const cxl_p2n_reg_t CXL_PSL_WED_An     = {0x0A0};
>  #define CXL_PSL_SPAP_Size_Shift 4
>  #define CXL_PSL_SPAP_V    0x0000000000000001ULL
>  
> +/****** CXL_PSL_Control ****************************************************/
> +#define CXL_PSL_Control_tb 0x0000000000000001ULL
> +
>  /****** CXL_PSL_DLCNTL *****************************************************/
>  #define CXL_PSL_DLCNTL_D (0x1ull << (63-28))
>  #define CXL_PSL_DLCNTL_C (0x1ull << (63-29))
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index fc938de..ea1a79f 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -360,6 +360,55 @@ static int init_implementation_adapter_regs(struct cxl *adapter, struct pci_dev
>  	return 0;
>  }
>  
> +#define TBSYNC_CNT(n) (((u64)n & 0x7) << (63-6))
> +#define _2048_250MHZ_CYCLES 1
> +
I'm not quite clear what these macros do or mean, and I'm also a little
bit uncomfortable with them just sitting in the middle of the file.

In particular, what does _2048_250MHZ_CYCLES mean?

> +static int cxl_setup_psl_timebase(struct cxl *adapter, struct pci_dev *dev)
> +{
> +	u64 psl_tb;
> +	int delta;
> +	unsigned int retry = 0;
> +	struct device_node *np;
> +
> +	if (!(np = pnv_pci_to_phb_node(dev)))
> +		return -ENODEV;
> +
> +	/* Do not fail when CAPP timebase sync is not supported by OPAL */
> +	of_node_get(np);
> +	if (! of_get_property(np, "ibm,capp-timebase-sync", NULL)) {
> +		of_node_put(np);
> +		pr_err("PSL: Timebase sync: OPAL support missing\n");
> +		return 0;
> +	}
> +	of_node_put(np);
Do we need to grab a reference to the node?
> +
> +	/*
> +	 * Setup PSL Timebase Control and Status register
> +	 * with the recommended Timebase Sync Count value
> +	 */
> +	cxl_p1_write(adapter, CXL_PSL_TB_CTLSTAT,
> +		     TBSYNC_CNT(2 * _2048_250MHZ_CYCLES));
> +
> +	/* Enable PSL Timebase */
> +	cxl_p1_write(adapter, CXL_PSL_Control, 0x0000000000000000);
> +	cxl_p1_write(adapter, CXL_PSL_Control, CXL_PSL_Control_tb);
> +
Is there a reason we need to write 0s to PSL control before we write the
timebase control value?

> +	/* Wait until CORE TB and PSL TB difference <= 16usecs */
> +	do {
> +		msleep(1);
> +		if (retry++ > 5) {
> +			pr_err("PSL: Timebase sync: giving up!\n");
> +			return -EIO;
> +		}
> +		psl_tb = cxl_p1_read(adapter, CXL_PSL_Timebase);
> +		delta = mftb() - psl_tb;
> +		if (delta < 0)
> +			delta = -delta;
> +	} while (cputime_to_usecs(delta) > 16);
> +
> +	return 0;
> +}
> +
>  static int init_implementation_afu_regs(struct cxl_afu *afu)
>  {
>  	/* read/write masks for this slice */
> @@ -952,9 +1001,12 @@ static struct cxl *cxl_alloc_adapter(struct pci_dev *dev)
>  	return adapter;
>  }
>  
> +#define CXL_PSL_ErrIVTE_tberror (0x1ull << (63-31))
> +
Again, could we put this somewhere else, rather than in the middle of
the file?
>  static int sanitise_adapter_regs(struct cxl *adapter)
>  {
> -	cxl_p1_write(adapter, CXL_PSL_ErrIVTE, 0x0000000000000000);
> +	/* Clear PSL tberror bit by writing 1 to it */
> +	cxl_p1_write(adapter, CXL_PSL_ErrIVTE, CXL_PSL_ErrIVTE_tberror);
>  	return cxl_tlb_slb_invalidate(adapter);
>  }
>  
> @@ -995,6 +1047,9 @@ static struct cxl *cxl_init_adapter(struct pci_dev *dev)
>  	if ((rc = pnv_phb_to_cxl(dev, OPAL_PHB_CAPI_MODE_CAPI)))
>  		goto err3;
>  
> +	if ((rc = cxl_setup_psl_timebase(adapter, dev)))
> +		goto err3;
> +
This bit will no longer apply cleanly. I *think* this should go in the
adapter config path (the part that's done on both setup and recovery).
>  	if ((rc = cxl_register_psl_err_irq(adapter)))
>  		goto err3;
>  

-- 
Regards,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 860 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20150815/4aa694f7/attachment-0001.sig>