[2/5] NX: Organize NX compression code to include 842 and gzip engines support

Message ID 1487569423.5897.6.camel@hbabu-laptop
State Superseded
Headers show

Commit Message

Haren Myneni Feb. 20, 2017, 5:43 a.m.
In addition to 842 compression, P9 NX also supports gzip compression.
So this patch creates nx-compress.c and reorg nx-842.c code so that
common functions that are needed for both 842 and gzip compression
will be moved in to nx-compress.c. This patch does not change the
actual functionality.

Signed-off-by: Haren Myneni <haren@us.ibm.com>
---
 hw/Makefile.inc  |    2 +-
 hw/nx-842.c      |   21 +++++++--------------
 hw/nx-compress.c |   34 ++++++++++++++++++++++++++++++++++
 hw/nx.c          |    2 +-
 include/nx.h     |    4 +++-
 5 files changed, 46 insertions(+), 17 deletions(-)
 create mode 100644 hw/nx-compress.c

Comments

Sukadev Bhattiprolu Feb. 21, 2017, 8:03 p.m. | #1
Haren Myneni [haren@linux.vnet.ibm.com] wrote:
> 
> In addition to 842 compression, P9 NX also supports gzip compression.
> So this patch creates nx-compress.c and reorg nx-842.c code so that
> common functions that are needed for both 842 and gzip compression
> will be moved in to nx-compress.c. This patch does not change the
> actual functionality.
> 
> Signed-off-by: Haren Myneni <haren@us.ibm.com>
> ---
>  hw/Makefile.inc  |    2 +-
>  hw/nx-842.c      |   21 +++++++--------------
>  hw/nx-compress.c |   34 ++++++++++++++++++++++++++++++++++
>  hw/nx.c          |    2 +-
>  include/nx.h     |    4 +++-
>  5 files changed, 46 insertions(+), 17 deletions(-)
>  create mode 100644 hw/nx-compress.c
> 
> diff --git a/hw/Makefile.inc b/hw/Makefile.inc
> index f2dc328..97c909e 100644
> --- a/hw/Makefile.inc
> +++ b/hw/Makefile.inc
> @@ -2,7 +2,7 @@
>  SUBDIRS += hw
>  HW_OBJS  = xscom.o chiptod.o gx.o cec.o lpc.o lpc-uart.o psi.o
>  HW_OBJS += homer.o slw.o occ.o fsi-master.o centaur.o
> -HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-842.o
> +HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-compress.o nx-842.o
>  HW_OBJS += p7ioc.o p7ioc-inits.o p7ioc-phb.o
>  HW_OBJS += phb3.o sfc-ctrl.o fake-rtc.o bt.o p8-i2c.o prd.o
>  HW_OBJS += dts.o lpc-rtc.o npu.o npu-hw-procedures.o xive.o phb4.o
> diff --git a/hw/nx-842.c b/hw/nx-842.c
> index f462f7a..3ccb549 100644
> --- a/hw/nx-842.c
> +++ b/hw/nx-842.c
> @@ -90,7 +90,7 @@ static int nx_cfg_842(u32 gcid, u64 xcfg)
>  	return rc;
>  }
> 
> -static int nx_cfg_dma(u32 gcid, u64 xcfg)
> +static int nx_cfg_842_dma(u32 gcid, u64 xcfg)
>  {
>  	u64 cfg;
>  	int rc;
> @@ -100,9 +100,9 @@ static int nx_cfg_dma(u32 gcid, u64 xcfg)
>  		return rc;
> 
>  	if (proc_gen == proc_gen_p8) {
> -		cfg = SETFIELD(NX_P8_DMA_CFG_842_COMPRESS_PREFETCH, cfg,
> +		cfg = SETFIELD(NX_DMA_CFG_842_COMPRESS_PREFETCH, cfg,
>  			       DMA_COMPRESS_PREFETCH);

If this is just a boolean field, how about s/DMA_COMPRESS_PREFETCH/true/?

> -		cfg = SETFIELD(NX_P8_DMA_CFG_842_DECOMPRESS_PREFETCH, cfg,
> +		cfg = SETFIELD(NX_DMA_CFG_842_DECOMPRESS_PREFETCH, cfg,
>  			       DMA_DECOMPRESS_PREFETCH);

ditto

Also, can you move this change into previous patch to make sure that patch
is bisect safe? It is unrelated to the code reorg anyway.

>  	}
> 
> @@ -131,7 +131,7 @@ static int nx_cfg_dma(u32 gcid, u64 xcfg)
>  	return rc;
>  }
> 
> -static int nx_cfg_ee(u32 gcid, u64 xcfg)
> +static int nx_cfg_842_ee(u32 gcid, u64 xcfg)
>  {
>  	u64 cfg;
>  	int rc;
> @@ -153,18 +153,11 @@ static int nx_cfg_ee(u32 gcid, u64 xcfg)
>  	return rc;
>  }
> 
> -void nx_create_842_node(struct dt_node *node)
> +void nx_enable_842(struct dt_node *node, u32 gcid, u32 pb_base)

I understand parameters are different, but the functionality is still
similar to the other nx_create*_node() functions (i.e it creates the
DT properties) How about leaving the name as is (nx_create_842_node())?

>  {
> -	u32 gcid;
> -	u32 pb_base;
>  	u64 cfg_dma, cfg_842, cfg_ee;
>  	int rc;
> 
> -	gcid = dt_get_chip_id(node);
> -	pb_base = dt_get_address(node, 0, NULL);
> -
> -	prlog(PR_INFO, "NX%d: 842 at 0x%x\n", gcid, pb_base);
> -
>  	if (dt_node_is_compatible(node, "ibm,power7-nx")) {
>  		cfg_dma = pb_base + NX_P7_DMA_CFG;
>  		cfg_842 = pb_base + NX_P7_842_CFG;
> @@ -178,7 +171,7 @@ void nx_create_842_node(struct dt_node *node)
>  		return;
>  	}
> 
> -	rc = nx_cfg_dma(gcid, cfg_dma);
> +	rc = nx_cfg_842_dma(gcid, cfg_dma);
>  	if (rc)
>  		return;
> 
> @@ -186,7 +179,7 @@ void nx_create_842_node(struct dt_node *node)
>  	if (rc)
>  		return;
> 
> -	rc = nx_cfg_ee(gcid, cfg_ee);
> +	rc = nx_cfg_842_ee(gcid, cfg_ee);
>  	if (rc)
>  		return;
> 
> diff --git a/hw/nx-compress.c b/hw/nx-compress.c
> new file mode 100644
> index 0000000..2ea2734
> --- /dev/null
> +++ b/hw/nx-compress.c
> @@ -0,0 +1,34 @@
> +/* Copyright 2015 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + * 	http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <skiboot.h>
> +#include <chip.h>
> +#include <xscom.h>
> +#include <io.h>
> +#include <cpu.h>
> +#include <nx.h>
> +
> +void nx_create_compress_node(struct dt_node *node)

Since this will create 842 and GZIP nodes in the follow on patch, maybe
call it nx_create_compress_nodes()?

> +{
> +	u32 gcid, pb_base;
> +
> +	gcid = dt_get_chip_id(node);
> +	pb_base = dt_get_address(node, 0, NULL);
> +
> +	prlog(PR_INFO, "NX%d: 842 at 0x%x\n", gcid, pb_base);
> +
> +	nx_enable_842(node, gcid, pb_base);
> +}
> diff --git a/hw/nx.c b/hw/nx.c
> index 83528d1..020c9e4 100644
> --- a/hw/nx.c
> +++ b/hw/nx.c
> @@ -29,6 +29,6 @@ void nx_init(void)
>  	dt_for_each_compatible(dt_root, node, "ibm,power-nx") {
>  		nx_create_rng_node(node);
>  		nx_create_crypto_node(node);
> -		nx_create_842_node(node);
> +		nx_create_compress_node(node);
>  	}
>  }
> diff --git a/include/nx.h b/include/nx.h
> index 4a67020..0a7b1b0 100644
> --- a/include/nx.h
> +++ b/include/nx.h
> @@ -378,7 +378,9 @@
> 
>  extern void nx_create_rng_node(struct dt_node *);
>  extern void nx_create_crypto_node(struct dt_node *);
> -extern void nx_create_842_node(struct dt_node *);
> +extern void nx_create_compress_node(struct dt_node *);
> +
> +extern void nx_enable_842(struct dt_node *, u32 gcid, u32 pb_base);
> 
>  extern void nx_init(void);
> 
> -- 
> 1.7.1
> 
>
Haren Myneni Feb. 21, 2017, 10:22 p.m. | #2
On 02/21/2017 12:03 PM, Sukadev Bhattiprolu wrote:
> Haren Myneni [haren@linux.vnet.ibm.com] wrote:
>>
>> In addition to 842 compression, P9 NX also supports gzip compression.
>> So this patch creates nx-compress.c and reorg nx-842.c code so that
>> common functions that are needed for both 842 and gzip compression
>> will be moved in to nx-compress.c. This patch does not change the
>> actual functionality.
>>
>> Signed-off-by: Haren Myneni <haren@us.ibm.com>
>> ---
>>  hw/Makefile.inc  |    2 +-
>>  hw/nx-842.c      |   21 +++++++--------------
>>  hw/nx-compress.c |   34 ++++++++++++++++++++++++++++++++++
>>  hw/nx.c          |    2 +-
>>  include/nx.h     |    4 +++-
>>  5 files changed, 46 insertions(+), 17 deletions(-)
>>  create mode 100644 hw/nx-compress.c
>>
>> diff --git a/hw/Makefile.inc b/hw/Makefile.inc
>> index f2dc328..97c909e 100644
>> --- a/hw/Makefile.inc
>> +++ b/hw/Makefile.inc
>> @@ -2,7 +2,7 @@
>>  SUBDIRS += hw
>>  HW_OBJS  = xscom.o chiptod.o gx.o cec.o lpc.o lpc-uart.o psi.o
>>  HW_OBJS += homer.o slw.o occ.o fsi-master.o centaur.o
>> -HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-842.o
>> +HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-compress.o nx-842.o
>>  HW_OBJS += p7ioc.o p7ioc-inits.o p7ioc-phb.o
>>  HW_OBJS += phb3.o sfc-ctrl.o fake-rtc.o bt.o p8-i2c.o prd.o
>>  HW_OBJS += dts.o lpc-rtc.o npu.o npu-hw-procedures.o xive.o phb4.o
>> diff --git a/hw/nx-842.c b/hw/nx-842.c
>> index f462f7a..3ccb549 100644
>> --- a/hw/nx-842.c
>> +++ b/hw/nx-842.c
>> @@ -90,7 +90,7 @@ static int nx_cfg_842(u32 gcid, u64 xcfg)
>>  	return rc;
>>  }
>>
>> -static int nx_cfg_dma(u32 gcid, u64 xcfg)
>> +static int nx_cfg_842_dma(u32 gcid, u64 xcfg)
>>  {
>>  	u64 cfg;
>>  	int rc;
>> @@ -100,9 +100,9 @@ static int nx_cfg_dma(u32 gcid, u64 xcfg)
>>  		return rc;
>>
>>  	if (proc_gen == proc_gen_p8) {
>> -		cfg = SETFIELD(NX_P8_DMA_CFG_842_COMPRESS_PREFETCH, cfg,
>> +		cfg = SETFIELD(NX_DMA_CFG_842_COMPRESS_PREFETCH, cfg,
>>  			       DMA_COMPRESS_PREFETCH);
> 
> If this is just a boolean field, how about s/DMA_COMPRESS_PREFETCH/true/?

I was following the same syntax with the existing code.  

> 
>> -		cfg = SETFIELD(NX_P8_DMA_CFG_842_DECOMPRESS_PREFETCH, cfg,
>> +		cfg = SETFIELD(NX_DMA_CFG_842_DECOMPRESS_PREFETCH, cfg,
>>  			       DMA_DECOMPRESS_PREFETCH);
> 
> ditto
> 
> Also, can you move this change into previous patch to make sure that patch
> is bisect safe? It is unrelated to the code reorg anyway.
> 
>>  	}
>>
>> @@ -131,7 +131,7 @@ static int nx_cfg_dma(u32 gcid, u64 xcfg)
>>  	return rc;
>>  }
>>
>> -static int nx_cfg_ee(u32 gcid, u64 xcfg)
>> +static int nx_cfg_842_ee(u32 gcid, u64 xcfg)
>>  {
>>  	u64 cfg;
>>  	int rc;
>> @@ -153,18 +153,11 @@ static int nx_cfg_ee(u32 gcid, u64 xcfg)
>>  	return rc;
>>  }
>>
>> -void nx_create_842_node(struct dt_node *node)
>> +void nx_enable_842(struct dt_node *node, u32 gcid, u32 pb_base)
> 
> I understand parameters are different, but the functionality is still
> similar to the other nx_create*_node() functions (i.e it creates the
> DT properties) How about leaving the name as is (nx_create_842_node())?

Also thought so. nx_create_*_node() functions are called from nx.c (top_level) for different functionalities (compress and encryption). These functions also enable 842 / gzip engines. Since parameters are different, used nx_enable*()  

> 
>>  {
>> -	u32 gcid;
>> -	u32 pb_base;
>>  	u64 cfg_dma, cfg_842, cfg_ee;
>>  	int rc;
>>
>> -	gcid = dt_get_chip_id(node);
>> -	pb_base = dt_get_address(node, 0, NULL);
>> -
>> -	prlog(PR_INFO, "NX%d: 842 at 0x%x\n", gcid, pb_base);
>> -
>>  	if (dt_node_is_compatible(node, "ibm,power7-nx")) {
>>  		cfg_dma = pb_base + NX_P7_DMA_CFG;
>>  		cfg_842 = pb_base + NX_P7_842_CFG;
>> @@ -178,7 +171,7 @@ void nx_create_842_node(struct dt_node *node)
>>  		return;
>>  	}
>>
>> -	rc = nx_cfg_dma(gcid, cfg_dma);
>> +	rc = nx_cfg_842_dma(gcid, cfg_dma);
>>  	if (rc)
>>  		return;
>>
>> @@ -186,7 +179,7 @@ void nx_create_842_node(struct dt_node *node)
>>  	if (rc)
>>  		return;
>>
>> -	rc = nx_cfg_ee(gcid, cfg_ee);
>> +	rc = nx_cfg_842_ee(gcid, cfg_ee);
>>  	if (rc)
>>  		return;
>>
>> diff --git a/hw/nx-compress.c b/hw/nx-compress.c
>> new file mode 100644
>> index 0000000..2ea2734
>> --- /dev/null
>> +++ b/hw/nx-compress.c
>> @@ -0,0 +1,34 @@
>> +/* Copyright 2015 IBM Corp.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at
>> + *
>> + * 	http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> + * implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include <skiboot.h>
>> +#include <chip.h>
>> +#include <xscom.h>
>> +#include <io.h>
>> +#include <cpu.h>
>> +#include <nx.h>
>> +
>> +void nx_create_compress_node(struct dt_node *node)
> 
> Since this will create 842 and GZIP nodes in the follow on patch, maybe
> call it nx_create_compress_nodes()?

NX provides compression, encryption and etc. In each functionality, several engines can be defined / sopported. So I think *compress_node() naming convention should be similar to *encrypt_node().
> 
>> +{
>> +	u32 gcid, pb_base;
>> +
>> +	gcid = dt_get_chip_id(node);
>> +	pb_base = dt_get_address(node, 0, NULL);
>> +
>> +	prlog(PR_INFO, "NX%d: 842 at 0x%x\n", gcid, pb_base);
>> +
>> +	nx_enable_842(node, gcid, pb_base);
>> +}
>> diff --git a/hw/nx.c b/hw/nx.c
>> index 83528d1..020c9e4 100644
>> --- a/hw/nx.c
>> +++ b/hw/nx.c
>> @@ -29,6 +29,6 @@ void nx_init(void)
>>  	dt_for_each_compatible(dt_root, node, "ibm,power-nx") {
>>  		nx_create_rng_node(node);
>>  		nx_create_crypto_node(node);
>> -		nx_create_842_node(node);
>> +		nx_create_compress_node(node);
>>  	}
>>  }
>> diff --git a/include/nx.h b/include/nx.h
>> index 4a67020..0a7b1b0 100644
>> --- a/include/nx.h
>> +++ b/include/nx.h
>> @@ -378,7 +378,9 @@
>>
>>  extern void nx_create_rng_node(struct dt_node *);
>>  extern void nx_create_crypto_node(struct dt_node *);
>> -extern void nx_create_842_node(struct dt_node *);
>> +extern void nx_create_compress_node(struct dt_node *);
>> +
>> +extern void nx_enable_842(struct dt_node *, u32 gcid, u32 pb_base);
>>
>>  extern void nx_init(void);
>>
>> -- 
>> 1.7.1
>>
>>
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>

Patch

diff --git a/hw/Makefile.inc b/hw/Makefile.inc
index f2dc328..97c909e 100644
--- a/hw/Makefile.inc
+++ b/hw/Makefile.inc
@@ -2,7 +2,7 @@ 
 SUBDIRS += hw
 HW_OBJS  = xscom.o chiptod.o gx.o cec.o lpc.o lpc-uart.o psi.o
 HW_OBJS += homer.o slw.o occ.o fsi-master.o centaur.o
-HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-842.o
+HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-compress.o nx-842.o
 HW_OBJS += p7ioc.o p7ioc-inits.o p7ioc-phb.o
 HW_OBJS += phb3.o sfc-ctrl.o fake-rtc.o bt.o p8-i2c.o prd.o
 HW_OBJS += dts.o lpc-rtc.o npu.o npu-hw-procedures.o xive.o phb4.o
diff --git a/hw/nx-842.c b/hw/nx-842.c
index f462f7a..3ccb549 100644
--- a/hw/nx-842.c
+++ b/hw/nx-842.c
@@ -90,7 +90,7 @@  static int nx_cfg_842(u32 gcid, u64 xcfg)
 	return rc;
 }
 
-static int nx_cfg_dma(u32 gcid, u64 xcfg)
+static int nx_cfg_842_dma(u32 gcid, u64 xcfg)
 {
 	u64 cfg;
 	int rc;
@@ -100,9 +100,9 @@  static int nx_cfg_dma(u32 gcid, u64 xcfg)
 		return rc;
 
 	if (proc_gen == proc_gen_p8) {
-		cfg = SETFIELD(NX_P8_DMA_CFG_842_COMPRESS_PREFETCH, cfg,
+		cfg = SETFIELD(NX_DMA_CFG_842_COMPRESS_PREFETCH, cfg,
 			       DMA_COMPRESS_PREFETCH);
-		cfg = SETFIELD(NX_P8_DMA_CFG_842_DECOMPRESS_PREFETCH, cfg,
+		cfg = SETFIELD(NX_DMA_CFG_842_DECOMPRESS_PREFETCH, cfg,
 			       DMA_DECOMPRESS_PREFETCH);
 	}
 
@@ -131,7 +131,7 @@  static int nx_cfg_dma(u32 gcid, u64 xcfg)
 	return rc;
 }
 
-static int nx_cfg_ee(u32 gcid, u64 xcfg)
+static int nx_cfg_842_ee(u32 gcid, u64 xcfg)
 {
 	u64 cfg;
 	int rc;
@@ -153,18 +153,11 @@  static int nx_cfg_ee(u32 gcid, u64 xcfg)
 	return rc;
 }
 
-void nx_create_842_node(struct dt_node *node)
+void nx_enable_842(struct dt_node *node, u32 gcid, u32 pb_base)
 {
-	u32 gcid;
-	u32 pb_base;
 	u64 cfg_dma, cfg_842, cfg_ee;
 	int rc;
 
-	gcid = dt_get_chip_id(node);
-	pb_base = dt_get_address(node, 0, NULL);
-
-	prlog(PR_INFO, "NX%d: 842 at 0x%x\n", gcid, pb_base);
-
 	if (dt_node_is_compatible(node, "ibm,power7-nx")) {
 		cfg_dma = pb_base + NX_P7_DMA_CFG;
 		cfg_842 = pb_base + NX_P7_842_CFG;
@@ -178,7 +171,7 @@  void nx_create_842_node(struct dt_node *node)
 		return;
 	}
 
-	rc = nx_cfg_dma(gcid, cfg_dma);
+	rc = nx_cfg_842_dma(gcid, cfg_dma);
 	if (rc)
 		return;
 
@@ -186,7 +179,7 @@  void nx_create_842_node(struct dt_node *node)
 	if (rc)
 		return;
 
-	rc = nx_cfg_ee(gcid, cfg_ee);
+	rc = nx_cfg_842_ee(gcid, cfg_ee);
 	if (rc)
 		return;
 
diff --git a/hw/nx-compress.c b/hw/nx-compress.c
new file mode 100644
index 0000000..2ea2734
--- /dev/null
+++ b/hw/nx-compress.c
@@ -0,0 +1,34 @@ 
+/* Copyright 2015 IBM Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * 	http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <skiboot.h>
+#include <chip.h>
+#include <xscom.h>
+#include <io.h>
+#include <cpu.h>
+#include <nx.h>
+
+void nx_create_compress_node(struct dt_node *node)
+{
+	u32 gcid, pb_base;
+
+	gcid = dt_get_chip_id(node);
+	pb_base = dt_get_address(node, 0, NULL);
+
+	prlog(PR_INFO, "NX%d: 842 at 0x%x\n", gcid, pb_base);
+
+	nx_enable_842(node, gcid, pb_base);
+}
diff --git a/hw/nx.c b/hw/nx.c
index 83528d1..020c9e4 100644
--- a/hw/nx.c
+++ b/hw/nx.c
@@ -29,6 +29,6 @@  void nx_init(void)
 	dt_for_each_compatible(dt_root, node, "ibm,power-nx") {
 		nx_create_rng_node(node);
 		nx_create_crypto_node(node);
-		nx_create_842_node(node);
+		nx_create_compress_node(node);
 	}
 }
diff --git a/include/nx.h b/include/nx.h
index 4a67020..0a7b1b0 100644
--- a/include/nx.h
+++ b/include/nx.h
@@ -378,7 +378,9 @@ 
 
 extern void nx_create_rng_node(struct dt_node *);
 extern void nx_create_crypto_node(struct dt_node *);
-extern void nx_create_842_node(struct dt_node *);
+extern void nx_create_compress_node(struct dt_node *);
+
+extern void nx_enable_842(struct dt_node *, u32 gcid, u32 pb_base);
 
 extern void nx_init(void);