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

[Dan Streetman]

On 2015-02-05 00:08, Stewart Smith wrote:
> 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

ok i'll clarify the naming

> 
>> +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?

yep, i'll update these and below

> 
> 
> 
>> +	/* 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)

no, i just don't have the P7+ doc, and i think it has different register 
addrs there and maybe some other minor differences.  I'll get the P7+ NX 
workbook and I can add support, shouldn't be difficult.

> 
>> 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.

Well, this msg is only printed when the coprocessor is fully configured, 
and we're specifically configuring it to be disabled (we need to at 
least configure its Coprocessor Type value to make it different from the 
842 CT value).  If any of the register config steps fail, it'll print 
out an error message, and then there is no guarantee that the crypto 
coprocessors are actually disabled - they'll be set to whatever they 
happen to default to and/or were set to before.  So printing out a msg 
that they are disabled when a register config failed doesn't seem 
correct - we should only print the error in configuring its register.

> 
>> +
>> +	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.

I didn't do the RNG driver, but sure I can update the doc.

> 
>> 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.

ok.

> 
> 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.

yep that is the node that i use to detect the NX coprocessors.  Should i 
change the 842 node naming to better match that?  e.g. compatible = 
"ibm,power-nx-842" (with the gcid appended for the node name e.g. 
"ibm,power-nx-842#1")