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.
Signed-off-by: Ruchika Gupta <[email protected]>
---
drivers/crypto/caam/ctrl.c | 17 +++++------
drivers/crypto/caam/regs.h | 71 +++++++++++++++++++++++++---------------------
2 files changed, 47 insertions(+), 41 deletions(-)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 1c38f86..5d8782e8 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -371,7 +371,7 @@ EXPORT_SYMBOL(caam_get_era);
static int caam_probe(struct platform_device *pdev)
{
int ret, ring, rspec, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
- u64 caam_id;
+ u32 caam_id;
struct device *dev;
struct device_node *nprop, *np;
struct caam_ctrl __iomem *ctrl;
@@ -380,7 +380,7 @@ static int caam_probe(struct platform_device *pdev)
#ifdef CONFIG_DEBUG_FS
struct caam_perfmon *perfmon;
#endif
- u64 cha_vid;
+ u32 cha_vid_ls;
ctrlpriv = kzalloc(sizeof(struct caam_drv_private), GFP_KERNEL);
if (!ctrlpriv)
@@ -456,8 +456,9 @@ static int caam_probe(struct platform_device *pdev)
}
/* 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);
if (ctrlpriv->qi_present) {
ctrlpriv->qi = (struct caam_queue_if __force *)&topregs->qi;
/* This is all that's required to physically enable QI */
@@ -471,13 +472,13 @@ static int caam_probe(struct platform_device *pdev)
return -ENOMEM;
}
- cha_vid = rd_reg64(&topregs->ctrl.perfmon.cha_id);
+ cha_vid_ls = rd_reg32(&topregs->ctrl.perfmon.cha_id_ls);
/*
* If SEC has RNG version >= 4 and RNG state handle has not been
* already instantiated, do RNG instantiation
*/
- if ((cha_vid & CHA_ID_RNG_MASK) >> CHA_ID_RNG_SHIFT >= 4) {
+ if ((cha_vid_ls & CHA_ID_LS_RNG_MASK) >> CHA_ID_LS_RNG_SHIFT >= 4) {
ctrlpriv->rng4_sh_init =
rd_reg32(&topregs->ctrl.r4tst[0].rdsta);
/*
@@ -531,10 +532,10 @@ static int caam_probe(struct platform_device *pdev)
/* NOTE: RTIC detection ought to go here, around Si time */
- caam_id = rd_reg64(&topregs->ctrl.perfmon.caam_id);
+ caam_id = rd_reg32(&topregs->ctrl.perfmon.caam_id_ms);
/* 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());
dev_info(dev, "job rings = %d, qi = %d\n",
ctrlpriv->total_jobrs, ctrlpriv->qi_present);
diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index cbde8b9..7bb898d 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -114,45 +114,45 @@ struct jr_outentry {
*/
/* Number of DECOs */
-#define CHA_NUM_DECONUM_SHIFT 56
-#define CHA_NUM_DECONUM_MASK (0xfull << CHA_NUM_DECONUM_SHIFT)
+#define CHA_NUM_MS_DECONUM_SHIFT 24
+#define CHA_NUM_MS_DECONUM_MASK (0xfull << CHA_NUM_MS_DECONUM_SHIFT)
/* CHA Version IDs */
-#define CHA_ID_AES_SHIFT 0
-#define CHA_ID_AES_MASK (0xfull << CHA_ID_AES_SHIFT)
+#define CHA_ID_LS_AES_SHIFT 0
+#define CHA_ID_LS_AES_MASK (0xfull << CHA_ID_LS_AES_SHIFT)
-#define CHA_ID_DES_SHIFT 4
-#define CHA_ID_DES_MASK (0xfull << CHA_ID_DES_SHIFT)
+#define CHA_ID_LS_DES_SHIFT 4
+#define CHA_ID_LS_DES_MASK (0xfull << CHA_ID_LS_DES_SHIFT)
-#define CHA_ID_ARC4_SHIFT 8
-#define CHA_ID_ARC4_MASK (0xfull << CHA_ID_ARC4_SHIFT)
+#define CHA_ID_LS_ARC4_SHIFT 8
+#define CHA_ID_LS_ARC4_MASK (0xfull << CHA_ID_LS_ARC4_SHIFT)
-#define CHA_ID_MD_SHIFT 12
-#define CHA_ID_MD_MASK (0xfull << CHA_ID_MD_SHIFT)
+#define CHA_ID_LS_MD_SHIFT 12
+#define CHA_ID_LS_MD_MASK (0xfull << CHA_ID_LS_MD_SHIFT)
-#define CHA_ID_RNG_SHIFT 16
-#define CHA_ID_RNG_MASK (0xfull << CHA_ID_RNG_SHIFT)
+#define CHA_ID_LS_RNG_SHIFT 16
+#define CHA_ID_LS_RNG_MASK (0xfull << CHA_ID_LS_RNG_SHIFT)
-#define CHA_ID_SNW8_SHIFT 20
-#define CHA_ID_SNW8_MASK (0xfull << CHA_ID_SNW8_SHIFT)
+#define CHA_ID_LS_SNW8_SHIFT 20
+#define CHA_ID_LS_SNW8_MASK (0xfull << CHA_ID_LS_SNW8_SHIFT)
-#define CHA_ID_KAS_SHIFT 24
-#define CHA_ID_KAS_MASK (0xfull << CHA_ID_KAS_SHIFT)
+#define CHA_ID_LS_KAS_SHIFT 24
+#define CHA_ID_LS_KAS_MASK (0xfull << CHA_ID_LS_KAS_SHIFT)
-#define CHA_ID_PK_SHIFT 28
-#define CHA_ID_PK_MASK (0xfull << CHA_ID_PK_SHIFT)
+#define CHA_ID_LS_PK_SHIFT 28
+#define CHA_ID_LS_PK_MASK (0xfull << CHA_ID_LS_PK_SHIFT)
-#define CHA_ID_CRC_SHIFT 32
-#define CHA_ID_CRC_MASK (0xfull << CHA_ID_CRC_SHIFT)
+#define CHA_ID_MS_CRC_SHIFT 0
+#define CHA_ID_MS_CRC_MASK (0xfull << CHA_ID_MS_CRC_SHIFT)
-#define CHA_ID_SNW9_SHIFT 36
-#define CHA_ID_SNW9_MASK (0xfull << CHA_ID_SNW9_SHIFT)
+#define CHA_ID_MS_SNW9_SHIFT 4
+#define CHA_ID_MS_SNW9_MASK (0xfull << CHA_ID_MS_SNW9_SHIFT)
-#define CHA_ID_DECO_SHIFT 56
-#define CHA_ID_DECO_MASK (0xfull << CHA_ID_DECO_SHIFT)
+#define CHA_ID_MS_DECO_SHIFT 24
+#define CHA_ID_MS_DECO_MASK (0xfull << CHA_ID_MS_DECO_SHIFT)
-#define CHA_ID_JR_SHIFT 60
-#define CHA_ID_JR_MASK (0xfull << CHA_ID_JR_SHIFT)
+#define CHA_ID_MS_JR_SHIFT 28
+#define CHA_ID_MS_JR_MASK (0xfull << CHA_ID_MS_JR_SHIFT)
struct sec_vid {
u16 ip_id;
@@ -172,10 +172,12 @@ struct caam_perfmon {
u64 rsvd[13];
/* CAAM Hardware Instantiation Parameters fa0-fbf */
- u64 cha_rev; /* CRNR - CHA Revision Number */
-#define CTPR_QI_SHIFT 57
-#define CTPR_QI_MASK (0x1ull << CTPR_QI_SHIFT)
- u64 comp_parms; /* CTPR - Compile Parameters Register */
+ u32 cha_rev_ms; /* CRNR - CHA Rev No. Most significant half*/
+ u32 cha_rev_ls; /* CRNR - CHA Rev No. Least significant half*/
+#define CTPR_MS_QI_SHIFT 25
+#define CTPR_MS_QI_MASK (0x1ull << CTPR_MS_QI_SHIFT)
+ u32 comp_parms_ms; /* CTPR - Compile Parameters Register */
+ u32 comp_parms_ls; /* CTPR - Compile Parameters Register */
u64 rsvd1[2];
/* CAAM Global Status fc0-fdf */
@@ -189,9 +191,12 @@ struct caam_perfmon {
/* Component Instantiation Parameters fe0-fff */
u32 rtic_id; /* RVID - RTIC Version ID */
u32 ccb_id; /* CCBVID - CCB Version ID */
- u64 cha_id; /* CHAVID - CHA Version ID */
- u64 cha_num; /* CHANUM - CHA Number */
- u64 caam_id; /* CAAMVID - CAAM Version ID */
+ u32 cha_id_ms; /* CHAVID - CHA Version ID Most Significant*/
+ u32 cha_id_ls; /* CHAVID - CHA Version ID Least Significant*/
+ u32 cha_num_ms; /* CHANUM - CHA Number Most Significant */
+ u32 cha_num_ls; /* CHANUM - CHA Number Least Significant*/
+ u32 caam_id_ms; /* CAAMVID - CAAM Version ID MS */
+ u32 caam_id_ls; /* CAAMVID - CAAM Version ID LS */
};
/* LIODN programming for DMA configuration */
--
1.8.1.4
On Tue, 29 Apr 2014 15:34:37 +0530
Ruchika Gupta <[email protected]> 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
> -----Original Message-----
> From: Kim Phillips [mailto:[email protected]]
> Sent: Friday, May 02, 2014 2:15 AM
> To: Gupta Ruchika-R66431
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH] crypto:caam - Modify width of few read only registers
>
> On Tue, 29 Apr 2014 15:34:37 +0530
> Ruchika Gupta <[email protected]> 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 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.
Ruchika
>
> Kim
On Tue, 6 May 2014 05:11:23 -0500
Gupta Ruchika-R66431 <[email protected]> wrote:
> > From: Kim Phillips [mailto:[email protected]]
> > Sent: Friday, May 02, 2014 2:15 AM
> >
> > On Tue, 29 Apr 2014 15:34:37 +0530
> > Ruchika Gupta <[email protected]> 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
Hi Kim,
> -----Original Message-----
> From: Kim Phillips [mailto:[email protected]]
> Sent: Wednesday, May 07, 2014 2:02 AM
> To: Gupta Ruchika-R66431
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH] crypto:caam - Modify width of few read only registers
>
> On Tue, 6 May 2014 05:11:23 -0500
> Gupta Ruchika-R66431 <[email protected]> wrote:
>
> > > From: Kim Phillips [mailto:[email protected]]
> > > Sent: Friday, May 02, 2014 2:15 AM
> > >
> > > On Tue, 29 Apr 2014 15:34:37 +0530
> > > Ruchika Gupta <[email protected]> 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
On Tue, 6 May 2014 23:09:15 -0500
Gupta Ruchika-R66431 <[email protected]> wrote:
> Hi Kim,
Hi Ruchika,
> > From: Kim Phillips [mailto:[email protected]]
> > Sent: Wednesday, May 07, 2014 2:02 AM
> >
> > On Tue, 6 May 2014 05:11:23 -0500
> > Gupta Ruchika-R66431 <[email protected]> wrote:
> >
> > > > From: Kim Phillips [mailto:[email protected]]
> > > > Sent: Friday, May 02, 2014 2:15 AM
> > > >
> > > > On Tue, 29 Apr 2014 15:34:37 +0530
> > > > Ruchika Gupta <[email protected]> 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
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
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:[email protected]]
> Sent: Thursday, May 08, 2014 5:25 AM
> To: Gupta Ruchika-R66431
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH] crypto:caam - Modify width of few read only registers
>
> On Tue, 6 May 2014 23:09:15 -0500
> Gupta Ruchika-R66431 <[email protected]> wrote:
>
> > Hi Kim,
>
> Hi Ruchika,
>
> > > From: Kim Phillips [mailto:[email protected]]
> > > Sent: Wednesday, May 07, 2014 2:02 AM
> > >
> > > On Tue, 6 May 2014 05:11:23 -0500
> > > Gupta Ruchika-R66431 <[email protected]> wrote:
> > >
> > > > > From: Kim Phillips [mailto:[email protected]]
> > > > > Sent: Friday, May 02, 2014 2:15 AM
> > > > >
> > > > > On Tue, 29 Apr 2014 15:34:37 +0530 Ruchika Gupta
> > > > > <[email protected]> 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
On Tue, 29 Apr 2014 15:34:37 +0530
Ruchika Gupta <[email protected]> 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
Hi Kim
> -----Original Message-----
> From: Kim Phillips [mailto:[email protected]]
> Sent: Thursday, June 12, 2014 4:23 AM
> To: Gupta Ruchika-R66431
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH] crypto:caam - Modify width of few read only registers
>
> On Tue, 29 Apr 2014 15:34:37 +0530
> Ruchika Gupta <[email protected]> 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
On Thu, 12 Jun 2014 04:56:14 -0500
Gupta Ruchika-R66431 <[email protected]> wrote:
> > From: Kim Phillips [mailto:[email protected]]
> > 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