From: Stewart Smith Subject: Re: [PATCH] crypto/nx: Initialize 842 high and normal RxFIFO control registers Date: Mon, 04 Jun 2018 14:08:46 +1000 Message-ID: <878t7v2nxt.fsf@linux.vnet.ibm.com> References: <1527789287.5945.23.camel@hbabu-laptop> <87h8mn2bta.fsf@linux.vnet.ibm.com> <5B117946.8010100@linux.vnet.ibm.com> <87bmcr2xj4.fsf@linux.vnet.ibm.com> <5B14A797.2050306@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain Cc: linuxppc-dev@lists.ozlabs.org, herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org To: Haren Myneni Return-path: In-Reply-To: <5B14A797.2050306@linux.vnet.ibm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: linux-crypto.vger.kernel.org Haren Myneni writes: > On 06/03/2018 05:41 PM, Stewart Smith wrote: >> Haren Myneni writes: >>> On 06/01/2018 12:41 AM, Stewart Smith wrote: >>>> Haren Myneni writes: >>>>> NX increments readOffset by FIFO size in receive FIFO control register >>>>> when CRB is read. But the index in RxFIFO has to match with the >>>>> corresponding entry in FIFO maintained by VAS in kernel. Otherwise NX >>>>> may be processing incorrect CRBs and can cause CRB timeout. >>>>> >>>>> VAS FIFO offset is 0 when the receive window is opened during >>>>> initialization. When the module is reloaded or in kexec boot, readOffset >>>>> in FIFO control register may not match with VAS entry. This patch adds >>>>> nx_coproc_init OPAL call to reset readOffset and queued entries in FIFO >>>>> control register for both high and normal FIFOs. >>>>> >>>>> Signed-off-by: Haren Myneni >>>>> >>>>> diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h >>>>> index d886a5b..ff61e4b 100644 >>>>> --- a/arch/powerpc/include/asm/opal-api.h >>>>> +++ b/arch/powerpc/include/asm/opal-api.h >>>>> @@ -206,7 +206,8 @@ >>>>> #define OPAL_NPU_TL_SET 161 >>>>> #define OPAL_PCI_GET_PBCQ_TUNNEL_BAR 164 >>>>> #define OPAL_PCI_SET_PBCQ_TUNNEL_BAR 165 >>>>> -#define OPAL_LAST 165 >>>>> +#define OPAL_NX_COPROC_INIT 167 >>>>> +#define OPAL_LAST 167 >>>>> >>>>> /* Device tree flags */ >>>>> >>>>> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h >>>>> index 7159e1a..d79eb82 100644 >>>>> --- a/arch/powerpc/include/asm/opal.h >>>>> +++ b/arch/powerpc/include/asm/opal.h >>>>> @@ -288,6 +288,7 @@ int64_t opal_imc_counters_init(uint32_t type, uint64_t address, >>>>> int opal_get_power_shift_ratio(u32 handle, int token, u32 *psr); >>>>> int opal_set_power_shift_ratio(u32 handle, int token, u32 psr); >>>>> int opal_sensor_group_clear(u32 group_hndl, int token); >>>>> +int opal_nx_coproc_init(uint32_t chip_id, uint32_t ct); >>>>> >>>>> s64 opal_signal_system_reset(s32 cpu); >>>>> >>>>> diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S >>>>> index 3da30c2..c7541a9 100644 >>>>> --- a/arch/powerpc/platforms/powernv/opal-wrappers.S >>>>> +++ b/arch/powerpc/platforms/powernv/opal-wrappers.S >>>>> @@ -325,3 +325,4 @@ OPAL_CALL(opal_npu_spa_clear_cache, OPAL_NPU_SPA_CLEAR_CACHE); >>>>> OPAL_CALL(opal_npu_tl_set, OPAL_NPU_TL_SET); >>>>> OPAL_CALL(opal_pci_get_pbcq_tunnel_bar, OPAL_PCI_GET_PBCQ_TUNNEL_BAR); >>>>> OPAL_CALL(opal_pci_set_pbcq_tunnel_bar, OPAL_PCI_SET_PBCQ_TUNNEL_BAR); >>>>> +OPAL_CALL(opal_nx_coproc_init, OPAL_NX_COPROC_INIT); >>>>> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c >>>>> index 48fbb41..5e13908 100644 >>>>> --- a/arch/powerpc/platforms/powernv/opal.c >>>>> +++ b/arch/powerpc/platforms/powernv/opal.c >>>>> @@ -1035,3 +1035,5 @@ void powernv_set_nmmu_ptcr(unsigned long ptcr) >>>>> EXPORT_SYMBOL_GPL(opal_int_set_mfrr); >>>>> EXPORT_SYMBOL_GPL(opal_int_eoi); >>>>> EXPORT_SYMBOL_GPL(opal_error_code); >>>>> +/* Export the below symbol for NX compression */ >>>>> +EXPORT_SYMBOL(opal_nx_coproc_init); >>>>> diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c >>>>> index 1e87637..6c4784d 100644 >>>>> --- a/drivers/crypto/nx/nx-842-powernv.c >>>>> +++ b/drivers/crypto/nx/nx-842-powernv.c >>>>> @@ -24,6 +24,8 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> +#include >>>>> >>>>> MODULE_LICENSE("GPL"); >>>>> MODULE_AUTHOR("Dan Streetman "); >>>>> @@ -803,9 +805,26 @@ static int __init vas_cfg_coproc_info(struct device_node *dn, int chip_id, >>>>> if (!coproc) >>>>> return -ENOMEM; >>>>> >>>>> - if (!strcmp(priority, "High")) >>>>> + if (!strcmp(priority, "High")) { >>>>> + /* >>>>> + * (lpid, pid, tid) combination has to be unique for each >>>>> + * coprocessor instance in the system. So to make it >>>>> + * unique, skiboot uses coprocessor type such as 842 or >>>>> + * GZIP for pid and provides this value to kernel in pid >>>>> + * device-tree property. >>>>> + * >>>>> + * Initialize each NX instance for both high and normal >>>>> + * priority FIFOs. >>>>> + */ >>>>> + ret = opal_nx_coproc_init(chip_id, pid); >>>>> + if (ret) { >>>>> + pr_err("Failed to initialize NX coproc: %d\n", ret); >>>>> + ret = opal_error_code(ret); >>>>> + goto err_out; >>>>> + } >>>>> + >>>>> coproc->ct = VAS_COP_TYPE_842_HIPRI; >>>> >>>> I think this should be called for all priority queues as it would be at >>>> least theoretically possible to only have Normal priority queues, in >>>> which case this patch wouldn't fix the problem. >>> >>> device tree exports separate nodes for high and normal priority FIFOs >>> per each NX instance. But NX init OPAL function is called once per >>> each NX instance when high-FIFO device node is parsed to reset high >>> and normal FIFO control registers. As you see in skiboot patch, resets >>> both priority registers. Thought we should minimize the number of OPAL >>> calla execution and also should have generic NX init OPAL call. We can >>> extend this call to reset default values for any other registers if >>> needed. >> >> This code does'nt do that though, it calls it for "high" priority, so if >> for whatever reason there was a DT without a "High" priority node there, >> it'd not call it. > > Skiboot exports high and normal FIFO nodes for each coprocessor type > per NX instance. We will never see only normal FIFO node without high > FIFO node. As we see in skiboot code, configure high FIFO and export > this node first, and then normal FIFO. In case if xscom read/ write > for high FIFO failed, we do not proceed for normal FIFO. Currently that's true, yes. But in the future it may not be. Imagine if we found a hardware bug that would mean disabling the 'High' priority one, or a future chip revision just had a single priority. -- Stewart Smith OPAL Architect, IBM.