[PATCH v2 1/2] powerpc: Define PVR value for POWER8NVL processor

[PATCH v2 1/2] powerpc: Define PVR value for POWER8NVL processor

[Philippe Bergheaud]

Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
---
V2:
  - New patch, added to patch set

 arch/powerpc/include/asm/reg.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index c4cb2ff..6a6de4a 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1174,6 +1174,7 @@
 #define PVR_970GX	0x0045
 #define PVR_POWER7p	0x004A
 #define PVR_POWER8E	0x004B
+#define PVR_POWER8NVL	0x004C
 #define PVR_POWER8	0x004D
 #define PVR_BE		0x0070
 #define PVR_PA6T	0x0090
-- 
2.1.0

[PATCH v2 2/2] cxl: Configure the PSL for two CAPI ports on POWER8NVL

[Philippe Bergheaud]

The POWER8NVL chip has two CAPI ports.  Configure the PSL to route
data to the port corresponding to the CAPP unit.

Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
---
V2:
  - Complete rewrite after Mikey's review

 drivers/misc/cxl/pci.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 0c6c17a1..924ba63 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -22,6 +22,7 @@
 #include <asm/pci-bridge.h> /* for struct pci_controller */
 #include <asm/pnv-pci.h>
 #include <asm/io.h>
+#include <asm/reg.h>
 
 #include "cxl.h"
 #include <misc/cxl.h>
@@ -340,12 +341,35 @@ static void dump_afu_descriptor(struct cxl_afu *afu)
 #undef show_reg
 }
 
+#define CAPP_UNIT0_ID 0xBA
+#define CAPP_UNIT1_ID 0XBE
+
+static u64 capp_unit_id(struct device_node *np)
+{
+	const __be32 *prop;
+	u64 phb_index;
+
+	/* For chips other than POWER8NVL, we only have CAPP 0,
+	 * irrespective of which PHB is used */
+	if (!pvr_version_is(PVR_POWER8NVL))
+		return CAPP_UNIT0_ID ;
+
+	/* For POWER8NVL, assume CAPP 0 is attached to PHB0 and
+	 * CAPP 1 is attached to PHB1 */
+	prop = of_get_property(np, "ibm,phb-index", NULL);
+	if (!prop)
+		return 0;
+	phb_index = be32_to_cpup(prop);
+	return phb_index ? CAPP_UNIT1_ID : CAPP_UNIT0_ID;
+}
+
 static int init_implementation_adapter_regs(struct cxl *adapter, struct pci_dev *dev)
 {
 	struct device_node *np;
 	const __be32 *prop;
 	u64 psl_dsnctl;
 	u64 chipid;
+	u64 cappunitid;
 
 	if (!(np = pnv_pci_get_phb_node(dev)))
 		return -ENODEV;
@@ -355,10 +379,15 @@ static int init_implementation_adapter_regs(struct cxl *adapter, struct pci_dev
 	if (!np)
 		return -ENODEV;
 	chipid = be32_to_cpup(prop);
+	cappunitid = capp_unit_id(np);
 	of_node_put(np);
+	if (!cappunitid)
+		return -ENODEV;
 
 	/* Tell PSL where to route data to */
-	psl_dsnctl = 0x02E8900002000000ULL | (chipid << (63-5));
+	psl_dsnctl = 0x0000900002000000ULL | (chipid << (63-5));
+	psl_dsnctl |= (cappunitid << (63-13));
+
 	cxl_p1_write(adapter, CXL_PSL_DSNDCTL, psl_dsnctl);
 	cxl_p1_write(adapter, CXL_PSL_RESLCKTO, 0x20000000200ULL);
 	/* snoop write mask */
-- 
2.1.0

Re: [PATCH v2 2/2] cxl: Configure the PSL for two CAPI ports on POWER8NVL

[Ian Munsie]

Acked-by: Ian Munsie <imunsie@au1.ibm.com>

Re: [PATCH v2 1/2] powerpc: Define PVR value for POWER8NVL processor

[Ian Munsie]

Acked-by: Ian Munsie <imunsie@au1.ibm.com>

Re: [v2,1/2] powerpc: Define PVR value for POWER8NVL processor

[Michael Ellerman]

On Wed, 2016-16-03 at 16:28:55 UTC, Philippe Bergheaud wrote:
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index c4cb2ff..6a6de4a 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -1174,6 +1174,7 @@
>  #define PVR_970GX	0x0045
>  #define PVR_POWER7p	0x004A
>  #define PVR_POWER8E	0x004B
> +#define PVR_POWER8NVL	0x004C
>  #define PVR_POWER8	0x004D
>  #define PVR_BE		0x0070
>  #define PVR_PA6T	0x0090

Oh wow, we have those defined separately to the cputable. That is "special" :/

Not your fault though.

cheers

Re: [v2,2/2] cxl: Configure the PSL for two CAPI ports on POWER8NVL

[Michael Ellerman]

On Wed, 2016-16-03 at 16:28:56 UTC, Philippe Bergheaud wrote:
> The POWER8NVL chip has two CAPI ports.  Configure the PSL to route
> data to the port corresponding to the CAPP unit.
> 
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index 0c6c17a1..924ba63 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -340,12 +341,35 @@ static void dump_afu_descriptor(struct cxl_afu *afu)
>  #undef show_reg
>  }
>  
> +#define CAPP_UNIT0_ID 0xBA
> +#define CAPP_UNIT1_ID 0XBE
> +
> +static u64 capp_unit_id(struct device_node *np)

It's always nice if function names contain a verb, eg. get_capp_unit_id().

> +{
> +	const __be32 *prop;
> +	u64 phb_index;
> +
> +	/* For chips other than POWER8NVL, we only have CAPP 0,
> +	 * irrespective of which PHB is used */

Can you use the standard block commenting style please, eg:

/*
 * For chips ..
 */

> +	if (!pvr_version_is(PVR_POWER8NVL))
> +		return CAPP_UNIT0_ID ;
                                    ^
				    don't need a space here

> +	/* For POWER8NVL, assume CAPP 0 is attached to PHB0 and
> +	 * CAPP 1 is attached to PHB1 */
> +	prop = of_get_property(np, "ibm,phb-index", NULL);

Please use of_property_read_u32().

> +	if (!prop)
> +		return 0;
> +	phb_index = be32_to_cpup(prop);

Some blank lines here would help readability.

> +	return phb_index ? CAPP_UNIT1_ID : CAPP_UNIT0_ID;

This logic doesn't quite match your comment above. This says that any non-zero
PHB index is CAPP1, whereas the comment says CAPP1 == PHB1.

> +}
> +
>  static int init_implementation_adapter_regs(struct cxl *adapter, struct pci_dev *dev)
>  {
>  	struct device_node *np;
>  	const __be32 *prop;
>  	u64 psl_dsnctl;
>  	u64 chipid;
> +	u64 cappunitid;

capp_unit_id ?

>  	if (!(np = pnv_pci_get_phb_node(dev)))
>  		return -ENODEV;
> @@ -355,10 +379,15 @@ static int init_implementation_adapter_regs(struct cxl *adapter, struct pci_dev
>  	if (!np)
>  		return -ENODEV;
>  	chipid = be32_to_cpup(prop);
> +	cappunitid = capp_unit_id(np);
>  	of_node_put(np);
> +	if (!cappunitid)
> +		return -ENODEV;

You probably want a pr_xxx() there don't you?

>  	/* Tell PSL where to route data to */
> -	psl_dsnctl = 0x02E8900002000000ULL | (chipid << (63-5));
> +	psl_dsnctl = 0x0000900002000000ULL | (chipid << (63-5));
> +	psl_dsnctl |= (cappunitid << (63-13));
> +
>  	cxl_p1_write(adapter, CXL_PSL_DSNDCTL, psl_dsnctl);
>  	cxl_p1_write(adapter, CXL_PSL_RESLCKTO, 0x20000000200ULL);
>  	/* snoop write mask */


cheers