RE: [PATCH] crypto:caam - Modify width of few read only registers
Hi Kim -Original Message- From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Thursday, June 12, 2014 4:23 AM To: Gupta Ruchika-R66431 Cc: linux-crypto@vger.kernel.org; herb...@gondor.apana.org.au Subject: Re: [PATCH] crypto:caam - Modify width of few read only registers On Tue, 29 Apr 2014 15:34:37 +0530 Ruchika Gupta ruchika.gu...@freescale.com wrote: Few read only registers like CHAVID, CTPR etc were wrongly defined as 64 bit registers. This functioned properly on the powerpc platforms. However ARM SoC's wouldn't function correctly if these registers are defined as 64 bit. So correcting the definition to two 32 bit registers. please rewrite, adding the details of the problem posted toward the end of this thread, e.g., what registers are affected, and how that renders MCFGR:DWT ineffective in this case. Ok. I will add the details in the commit message. /* Check to see if QI present. If so, enable */ - ctrlpriv-qi_present = !!(rd_reg64(topregs-ctrl.perfmon.comp_parms) - CTPR_QI_MASK); + ctrlpriv-qi_present = + !!(rd_reg32(topregs-ctrl.perfmon.comp_parms_ms) + CTPR_MS_QI_MASK); alignment Ok. I will correct it. /* Report alive for developer to see */ - dev_info(dev, device ID = 0x%016llx (Era %d)\n, caam_id, + dev_info(dev, device ID = 0x%08x (Era %d)\n, caam_id, caam_get_era()); Why are we dropping the upper 32 bits here? The upper 32 bit contain the IP ID of SEC, the major number and the minor number while the lower 32 bits have the details of the compile option, integration and configuration options of SEC. So device ID is actually contained only in the most significant 32 bits which are being printed here. Ruchika Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto:caam - Modify width of few read only registers
On Thu, 12 Jun 2014 04:56:14 -0500 Gupta Ruchika-R66431 ruchika.gu...@freescale.com wrote: From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Thursday, June 12, 2014 4:23 AM /* Check to see if QI present. If so, enable */ - ctrlpriv-qi_present = !!(rd_reg64(topregs-ctrl.perfmon.comp_parms) - CTPR_QI_MASK); + ctrlpriv-qi_present = + !!(rd_reg32(topregs-ctrl.perfmon.comp_parms_ms) + CTPR_MS_QI_MASK); alignment Ok. I will correct it. /* Report alive for developer to see */ - dev_info(dev, device ID = 0x%016llx (Era %d)\n, caam_id, + dev_info(dev, device ID = 0x%08x (Era %d)\n, caam_id, caam_get_era()); Why are we dropping the upper 32 bits here? The upper 32 bit contain the IP ID of SEC, the major number and the minor number while the lower 32 bits have the details of the compile option, integration and configuration options of SEC. So device ID is actually contained only in the most significant 32 bits which are being printed here. that may be true, but you're changing the driver to not display information it previously did, in a seemingly totally unrelated manner. This is a regression IMO - if you want the compile options specifically labelled in the display, then do that, but don't start hiding information from the user just because the h/w didn't pass an endianness change properly. Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto:caam - Modify width of few read only registers
On Tue, 29 Apr 2014 15:34:37 +0530 Ruchika Gupta ruchika.gu...@freescale.com wrote: Few read only registers like CHAVID, CTPR etc were wrongly defined as 64 bit registers. This functioned properly on the powerpc platforms. However ARM SoC's wouldn't function correctly if these registers are defined as 64 bit. So correcting the definition to two 32 bit registers. please rewrite, adding the details of the problem posted toward the end of this thread, e.g., what registers are affected, and how that renders MCFGR:DWT ineffective in this case. /* Check to see if QI present. If so, enable */ - ctrlpriv-qi_present = !!(rd_reg64(topregs-ctrl.perfmon.comp_parms) - CTPR_QI_MASK); + ctrlpriv-qi_present = + !!(rd_reg32(topregs-ctrl.perfmon.comp_parms_ms) + CTPR_MS_QI_MASK); alignment /* Report alive for developer to see */ - dev_info(dev, device ID = 0x%016llx (Era %d)\n, caam_id, + dev_info(dev, device ID = 0x%08x (Era %d)\n, caam_id, caam_get_era()); Why are we dropping the upper 32 bits here? Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] crypto:caam - Modify width of few read only registers
Hi Kim, I contacted the Hardware folks and below is the statement from them : Unfortunately setting the DWT bit will also affect the operation of job descriptors, so I don't think that is a viable option. It looks like you will have to change the software to access all 32-bit registers as 32-bit quantities, even if two 32-bit registers appear to be two halves of a 64-bit register. If you do that it will work correctly on both big-endian and little-endian SoCs. Regards, Ruchika -Original Message- From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Thursday, May 08, 2014 5:25 AM To: Gupta Ruchika-R66431 Cc: linux-crypto@vger.kernel.org; herb...@gondor.apana.org.au Subject: Re: [PATCH] crypto:caam - Modify width of few read only registers On Tue, 6 May 2014 23:09:15 -0500 Gupta Ruchika-R66431 ruchika.gu...@freescale.com wrote: Hi Kim, Hi Ruchika, From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Wednesday, May 07, 2014 2:02 AM On Tue, 6 May 2014 05:11:23 -0500 Gupta Ruchika-R66431 ruchika.gu...@freescale.com wrote: From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Friday, May 02, 2014 2:15 AM On Tue, 29 Apr 2014 15:34:37 +0530 Ruchika Gupta ruchika.gu...@freescale.com wrote: Few read only registers like CHAVID, CTPR etc were wrongly defined as 64 bit registers. This functioned properly on the powerpc platforms. However ARM SoC's wouldn't function correctly if these registers are defined as 64 bit. why wouldn't they function correctly? The SEC IP guide states these registers as 2 32 bit registers. So register definition in I'm looking at LS2100A's SEC reference manual, it clearly has the CHAVID defined as one, single 64-bit register. What are you looking at? In the first version of guide they were defined as 64 bit. They were later changed to 32 bit once issue was reported while testing on emulator. Latest guide of LS2100 has them modified. A register width column has also been added in the memory map now. I love how they try to cover up h/w bugs by amending the documentation... crypto code should also have them defined as 32 bit registers. Defining them as 64 bit in this case would be incorrect. Endianness of the CAAM IP varies with core's endiannes. In ARM SoC's , CAAM block is also little endian. So in case the 2 - 32 bit registers are treated as a 64 bit register, the result would be word swapped as compared to powerpc platforms. As a result, the reads won't return the right result. For eg. For the 2 32 bit registers CHAVID_MS(at address 0x0) and CHAVID_LS(address 0x4) , if core reads them as 64 bit word, In powerpc (big endian) platform - CHAVID_MS would be available in most significant portion of the 64 bit word. CHAVID_LS would be the in least significant portion. This is as expected. In ARM (little endian) platform, 64 bit read would result in - CHAVID_MS in Least significant portion of the word and CHAVID_LS in the most significant location. This result is word swapped and the value read wouldn't be correct. hmm, have you looked at using the DWT Double Word Transpose bit in the MCFGR? I am not able to locate this bit in MCFGR. It's bit 19: Double Word Transpose. Setting this bit affects whether the two words within a Dword are transposed when a double-word register is accessed, ... However there are few swapping options present in Job ring configuration and QICTL registers. Are you referring to these ? no. Plus, those don't sound relevant to accessing CHAVID... Since these are 32 bit registers by nature, shouldn't we just treat them as 32 bit instead of enabling the swapping option . depends on the definition of 'treat': I'd rather still use the superior 64- bit accessors on all possible arches, if we can get them to work. Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto:caam - Modify width of few read only registers
On Thursday, May 08, 2014 at 01:54:42 AM, Kim Phillips wrote: [...] In the first version of guide they were defined as 64 bit. They were later changed to 32 bit once issue was reported while testing on emulator. Latest guide of LS2100 has them modified. A register width column has also been added in the memory map now. I love how they try to cover up h/w bugs by amending the documentation... Typical, yes :-( [...] Since these are 32 bit registers by nature, shouldn't we just treat them as 32 bit instead of enabling the swapping option . depends on the definition of 'treat': I'd rather still use the superior 64-bit accessors on all possible arches, if we can get them to work. Was there any resolution for this problem ? Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto:caam - Modify width of few read only registers
On Tue, 6 May 2014 23:09:15 -0500 Gupta Ruchika-R66431 ruchika.gu...@freescale.com wrote: Hi Kim, Hi Ruchika, From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Wednesday, May 07, 2014 2:02 AM On Tue, 6 May 2014 05:11:23 -0500 Gupta Ruchika-R66431 ruchika.gu...@freescale.com wrote: From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Friday, May 02, 2014 2:15 AM On Tue, 29 Apr 2014 15:34:37 +0530 Ruchika Gupta ruchika.gu...@freescale.com wrote: Few read only registers like CHAVID, CTPR etc were wrongly defined as 64 bit registers. This functioned properly on the powerpc platforms. However ARM SoC's wouldn't function correctly if these registers are defined as 64 bit. why wouldn't they function correctly? The SEC IP guide states these registers as 2 32 bit registers. So register definition in I'm looking at LS2100A's SEC reference manual, it clearly has the CHAVID defined as one, single 64-bit register. What are you looking at? In the first version of guide they were defined as 64 bit. They were later changed to 32 bit once issue was reported while testing on emulator. Latest guide of LS2100 has them modified. A register width column has also been added in the memory map now. I love how they try to cover up h/w bugs by amending the documentation... crypto code should also have them defined as 32 bit registers. Defining them as 64 bit in this case would be incorrect. Endianness of the CAAM IP varies with core's endiannes. In ARM SoC's , CAAM block is also little endian. So in case the 2 - 32 bit registers are treated as a 64 bit register, the result would be word swapped as compared to powerpc platforms. As a result, the reads won't return the right result. For eg. For the 2 32 bit registers CHAVID_MS(at address 0x0) and CHAVID_LS(address 0x4) , if core reads them as 64 bit word, In powerpc (big endian) platform - CHAVID_MS would be available in most significant portion of the 64 bit word. CHAVID_LS would be the in least significant portion. This is as expected. In ARM (little endian) platform, 64 bit read would result in - CHAVID_MS in Least significant portion of the word and CHAVID_LS in the most significant location. This result is word swapped and the value read wouldn't be correct. hmm, have you looked at using the DWT Double Word Transpose bit in the MCFGR? I am not able to locate this bit in MCFGR. It's bit 19: Double Word Transpose. Setting this bit affects whether the two words within a Dword are transposed when a double-word register is accessed, ... However there are few swapping options present in Job ring configuration and QICTL registers. Are you referring to these ? no. Plus, those don't sound relevant to accessing CHAVID... Since these are 32 bit registers by nature, shouldn't we just treat them as 32 bit instead of enabling the swapping option . depends on the definition of 'treat': I'd rather still use the superior 64-bit accessors on all possible arches, if we can get them to work. Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto:caam - Modify width of few read only registers
On Tue, 6 May 2014 05:11:23 -0500 Gupta Ruchika-R66431 ruchika.gu...@freescale.com wrote: From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Friday, May 02, 2014 2:15 AM On Tue, 29 Apr 2014 15:34:37 +0530 Ruchika Gupta ruchika.gu...@freescale.com wrote: Few read only registers like CHAVID, CTPR etc were wrongly defined as 64 bit registers. This functioned properly on the powerpc platforms. However ARM SoC's wouldn't function correctly if these registers are defined as 64 bit. why wouldn't they function correctly? The SEC IP guide states these registers as 2 32 bit registers. So register definition in I'm looking at LS2100A's SEC reference manual, it clearly has the CHAVID defined as one, single 64-bit register. What are you looking at? crypto code should also have them defined as 32 bit registers. Defining them as 64 bit in this case would be incorrect. Endianness of the CAAM IP varies with core's endiannes. In ARM SoC's , CAAM block is also little endian. So in case the 2 - 32 bit registers are treated as a 64 bit register, the result would be word swapped as compared to powerpc platforms. As a result, the reads won't return the right result. For eg. For the 2 32 bit registers CHAVID_MS(at address 0x0) and CHAVID_LS(address 0x4) , if core reads them as 64 bit word, In powerpc (big endian) platform - CHAVID_MS would be available in most significant portion of the 64 bit word. CHAVID_LS would be the in least significant portion. This is as expected. In ARM (little endian) platform, 64 bit read would result in - CHAVID_MS in Least significant portion of the word and CHAVID_LS in the most significant location. This result is word swapped and the value read wouldn't be correct. hmm, have you looked at using the DWT Double Word Transpose bit in the MCFGR? Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] crypto:caam - Modify width of few read only registers
Hi Kim, -Original Message- From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Wednesday, May 07, 2014 2:02 AM To: Gupta Ruchika-R66431 Cc: linux-crypto@vger.kernel.org; herb...@gondor.apana.org.au Subject: Re: [PATCH] crypto:caam - Modify width of few read only registers On Tue, 6 May 2014 05:11:23 -0500 Gupta Ruchika-R66431 ruchika.gu...@freescale.com wrote: From: Kim Phillips [mailto:kim.phill...@freescale.com] Sent: Friday, May 02, 2014 2:15 AM On Tue, 29 Apr 2014 15:34:37 +0530 Ruchika Gupta ruchika.gu...@freescale.com wrote: Few read only registers like CHAVID, CTPR etc were wrongly defined as 64 bit registers. This functioned properly on the powerpc platforms. However ARM SoC's wouldn't function correctly if these registers are defined as 64 bit. why wouldn't they function correctly? The SEC IP guide states these registers as 2 32 bit registers. So register definition in I'm looking at LS2100A's SEC reference manual, it clearly has the CHAVID defined as one, single 64-bit register. What are you looking at? In the first version of guide they were defined as 64 bit. They were later changed to 32 bit once issue was reported while testing on emulator. Latest guide of LS2100 has them modified. A register width column has also been added in the memory map now. crypto code should also have them defined as 32 bit registers. Defining them as 64 bit in this case would be incorrect. Endianness of the CAAM IP varies with core's endiannes. In ARM SoC's , CAAM block is also little endian. So in case the 2 - 32 bit registers are treated as a 64 bit register, the result would be word swapped as compared to powerpc platforms. As a result, the reads won't return the right result. For eg. For the 2 32 bit registers CHAVID_MS(at address 0x0) and CHAVID_LS(address 0x4) , if core reads them as 64 bit word, In powerpc (big endian) platform - CHAVID_MS would be available in most significant portion of the 64 bit word. CHAVID_LS would be the in least significant portion. This is as expected. In ARM (little endian) platform, 64 bit read would result in - CHAVID_MS in Least significant portion of the word and CHAVID_LS in the most significant location. This result is word swapped and the value read wouldn't be correct. hmm, have you looked at using the DWT Double Word Transpose bit in the MCFGR? I am not able to locate this bit in MCFGR. However there are few swapping options present in Job ring configuration and QICTL registers. Are you referring to these ? Since these are 32 bit registers by nature, shouldn't we just treat them as 32 bit instead of enabling the swapping option . Ruchika Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto:caam - Modify width of few read only registers
On Tue, 29 Apr 2014 15:34:37 +0530 Ruchika Gupta ruchika.gu...@freescale.com wrote: Few read only registers like CHAVID, CTPR etc were wrongly defined as 64 bit registers. This functioned properly on the powerpc platforms. However ARM SoC's wouldn't function correctly if these registers are defined as 64 bit. why wouldn't they function correctly? Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
