[Skiboot] [PATCHv2] add nx-842 coproc support

[Stewart Smith]

A couple of quick thoughts:

Dan Streetman <ddstreet at us.ibm.com> writes:
> Add support for the 842 hw memory compression engine in the NX Coprocessor.
> This moves the existing RNG support into its own nx-rng.c file, adds 842
> support in a nx-842.c file, and creates a nx-crypto.c file to configure and
> disable the crypto engines (which are not supported yet).
>
> New nodes are created for each 842 engine found.  This does not actually
> process any of the data or drive the 842 engines, it only configures
> registers to set up and enable/disable the engines appropriately, and
> creates new nodes so the OS can drive the 842 engines.
>
> diff --git a/hw/nx-842.c b/hw/nx-842.c
> new file mode 100644
> index 0000000..0332e35
> --- /dev/null
> +++ b/hw/nx-842.c
> @@ -0,0 +1,202 @@
<snip>
> +#include <skiboot.h>
> +#include <xscom.h>
> +#include <io.h>
> +#include <cpu.h>
> +#include <nx.h>
> +
> +static u64 nx_842_ci = 1;

This variable name isn't obvious as to what it's used for

> +static int nx_cfg_842(u32 gcid, u64 xcfg, u64 instance)
> +{
> +	u64 cfg, ci, ct;
> +	int rc;
> +
> +	if (instance > NX_P8_842_CFG_CI_MAX) {
> +		prerror("NX%d: ERROR: 842 CI %u exceeds max %u\n",
> +				gcid, (unsigned int)instance,
> +				NX_P8_842_CFG_CI_MAX);
> +		return OPAL_INTERNAL_ERROR;
> +	}
> +
> +	rc = xscom_read(gcid, xcfg, &cfg);
> +	if (rc)
> +		return rc;
> +
> +	ct = GETFIELD(NX_P8_842_CFG_CT, cfg);
> +	if (!ct)
> +		printf("NX%d:   842 CT set to %u\n", gcid, NX_CT_842);
> +	else if (ct == NX_CT_842)
> +		printf("NX%d:   842 CT already set to %u\n", gcid, NX_CT_842);
> +	else
> +		printf("NX%d:   842 CT already set to %u, changing to %u\n",
> +				gcid, (unsigned int)ct, NX_CT_842);

should these be prlog(PR_TRACE or PR_DEBUG instead?



> +	/* Coprocessor Instance must be shifted left.
> +	 * See hw doc Section 5.5.1.
> +	 */
> +	ci = GETFIELD(NX_P8_842_CFG_CI, cfg) >> NX_P8_842_CFG_CI_LSHIFT;
> +	if (!ci)
> +		printf("NX%d:   842 CI set to %u\n", gcid,
> +				(unsigned int)instance);
> +	else if (ci == instance)
> +		printf("NX%d:   842 CI already set to %u\n", gcid,
> +				(unsigned int)instance);
> +	else
> +		printf("NX%d:   842 CI already set to %u, changing to %u\n",
> +				gcid, (unsigned int)ci, (unsigned int)instance);
> +	ci = instance;

same here

> +	cfg = SETFIELD(NX_P8_842_CFG_CI, cfg,
> +			ci << NX_P8_842_CFG_CI_LSHIFT);
> +
> +	/* Enable all functions */
> +	cfg = SETFIELD(NX_P8_842_CFG_FC_ENABLE, cfg, 0x1f);
> +
> +	/* Enable this engine */
> +	cfg |= NX_P8_842_CFG_ENABLE;
> +
> +	rc = xscom_write(gcid, xcfg, cfg);
> +	if (rc)
> +		prerror("NX%d: ERROR: 842 CT %u CI %u config failure %d\n",
> +				gcid, (unsigned int)ct, (unsigned int)ci, rc);
> +	else
> +		printf("NX%d:   842 Config 0x%016lx\n",
> +				gcid, (unsigned long)cfg);

PR_INFO instead?

> +	if (dt_node_is_compatible(node, "ibm,power7-nx")) {
> +		prerror("NX%d: ERROR: 842 not supported on Power7\n", gcid);
> +		return;

out of interest, is there enough hardware differences that it's a
problem to support on P7? (probably less of an issue as we get more and
more P8s for dev/test)

> diff --git a/hw/nx-crypto.c b/hw/nx-crypto.c
> new file mode 100644
> index 0000000..0683fc0
> --- /dev/null
> +++ b/hw/nx-crypto.c
> @@ -0,0 +1,213 @@
<snip>
> +static int nx_cfg_sym(u32 gcid, u64 xcfg, u64 instance)
> +{
> +	u64 cfg, ci, ct;
> +	int rc;
> +
> +	if (instance > NX_P8_SYM_CFG_CI_MAX) {
> +		prerror("NX%d: ERROR: SYM CI %u exceeds max %u\n",
> +				gcid, (unsigned int)instance,
> +				NX_P8_SYM_CFG_CI_MAX);
> +		return OPAL_INTERNAL_ERROR;
> +	}
<snip>
> +	rc = nx_cfg_sym(gcid, cfg_sym, nx_sym_ci++);
> +	if (rc)
> +		return;
> +
> +	rc = nx_cfg_asym(gcid, cfg_asym, nx_asym_ci++);
> +	if (rc)
> +		return;
> +
> +	rc = nx_cfg_ee(gcid, cfg_ee);
> +	if (rc)
> +		return;
> +
> +	printf("NX%d: Crypto Coprocessors Disabled (not supported)\n", gcid);
> +}

from the look of the flow of things, in any case where cfg fails, we
will *not* print the message saying the coprocessor is disabled.

> +
> +	dt_add_property_strings(rng, "compatible", "ibm,power-rng");
> +	dt_add_property_cells(rng, "reg", hi32(rng_addr), lo32(rng_addr),
> +			hi32(rng_len), lo32(rng_len));
> +	dt_add_property_cells(rng, "ibm,chip-id", gcid);
> +}

It would also be great to document this in doc/device-tree/ too.

> diff --git a/hw/nx.c b/hw/nx.c
> index 8f42717..83528d1 100644
> --- a/hw/nx.c
> +++ b/hw/nx.c
>  void nx_init(void)
>  {
>  	struct dt_node *node;
>
> -	dt_for_each_compatible(dt_root, node, "ibm,power-nx")
> -		nx_create_node(node);
> +	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);
> +	}

Adding to doc/device-tree/ on how 842 is presented to the OS would be
excellent.

We already have (from a dt on a p8):

                nx at 2010000 {
                        reg = <0x2010000 0x4000>;
                        phandle = <0x68>;
                        linux,phandle = <0x68>;
                        compatible = "ibm,power-nx", "ibm,power8-nx";
                };

and we're currently exposing this to the OS, so we should document
everything to ensure we a) get it right and b) don't create something
that's confusing.