Re: [PATCH 3/5] crypto:s5p-sss: Add support for SSS module on Exynos5
From: Tomasz Figa <hidden>
Date: 2014-01-08 00:29:23
Also in:
linux-samsung-soc, lkml
Hi Naveen, Please see my comments inline. On Tuesday 07 of January 2014 17:21:47 Naveen Krishna Ch wrote:
The differences between SSS modules on S5PV210 and Exynos5 (AFA the driver supports) 1. AES register are at an offset of 0x200 on Exynos5 2. hash interrupt is no longer needed on Exynos5
What about Exynos 4 SoCs? Do they have S5PV210- or Exynos5-style security block?
quoted hunk ↗ jump to hunk
This patch adds code needed to address the above changes. Signed-off-by: Naveen Krishna Ch <redacted> CC: Herbert Xu <herbert@gondor.apana.org.au> CC: David S. Miller <davem@davemloft.net> CC: Vladimir Zapolskiy <redacted> TO: <redacted> CC: <redacted> --- drivers/crypto/s5p-sss.c | 59 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 18 deletions(-)diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c index dcb9fc1..a11cd0e 100644 --- a/drivers/crypto/s5p-sss.c +++ b/drivers/crypto/s5p-sss.c@@ -106,7 +106,7 @@ #define SSS_REG_FCPKDMAO 0x005C /* AES registers */ -#define SSS_REG_AES_CONTROL 0x4000 +#define SSS_REG_AES_CONTROL(dev) ((dev)->aes_offset + 0x00)
This is ugly. Please consider using a variant struct to store addresses of registers that depend on IP version.
quoted hunk ↗ jump to hunk
#define SSS_AES_BYTESWAP_DI _BIT(11) #define SSS_AES_BYTESWAP_DO _BIT(10) #define SSS_AES_BYTESWAP_IV _BIT(9)@@ -122,21 +122,26 @@ #define SSS_AES_CHAIN_MODE_CTR _SBF(1, 0x02) #define SSS_AES_MODE_DECRYPT _BIT(0) -#define SSS_REG_AES_STATUS 0x4004 +#define SSS_REG_AES_STATUS(dev) ((dev)->aes_offset + 0x04)
Ditto.
#define SSS_AES_BUSY _BIT(2) #define SSS_AES_INPUT_READY _BIT(1) #define SSS_AES_OUTPUT_READY _BIT(0) -#define SSS_REG_AES_IN_DATA(s) (0x4010 + (s << 2)) -#define SSS_REG_AES_OUT_DATA(s) (0x4020 + (s << 2)) -#define SSS_REG_AES_IV_DATA(s) (0x4030 + (s << 2)) -#define SSS_REG_AES_CNT_DATA(s) (0x4040 + (s << 2)) -#define SSS_REG_AES_KEY_DATA(s) (0x4080 + (s << 2)) +#define SSS_REG_AES_IN_DATA(dev, s) ((dev)->aes_offset + 0x10 + (s << 2)) +#define SSS_REG_AES_OUT_DATA(dev, s) ((dev)->aes_offset + 0x20 + (s << 2)) +#define SSS_REG_AES_IV_DATA(dev, s) ((dev)->aes_offset + 0x30 + (s << 2)) +#define SSS_REG_AES_CNT_DATA(dev, s) ((dev)->aes_offset + 0x40 + (s << 2)) +#define SSS_REG_AES_KEY_DATA(dev, s) ((dev)->aes_offset + 0x80 + (s << 2))
Ditto.
quoted hunk ↗ jump to hunk
#define SSS_REG(dev, reg) ((dev)->ioaddr + (SSS_REG_##reg)) #define SSS_READ(dev, reg) __raw_readl(SSS_REG(dev, reg)) #define SSS_WRITE(dev, reg, val) __raw_writel((val), SSS_REG(dev, reg)) +#define SSS_AES_REG(dev, reg) ((dev)->ioaddr + (SSS_REG_##reg(dev))) +#define SSS_AES_READ(dev, reg) __raw_readl(SSS_AES_REG(dev, reg)) +#define SSS_AES_WRITE(dev, reg, val) __raw_writel((val), \ + SSS_AES_REG(dev, reg)) + /* HW engine modes */ #define FLAGS_AES_DECRYPT _BIT(0) #define FLAGS_AES_MODE_MASK _SBF(1, 0x03)@@ -177,6 +182,13 @@ struct s5p_aes_dev { /* To support SSS versions across Samsung SoCs */ unsigned int version; + + /* + * Register banks corresponding to various algorithms + * (Ex: AES, TDES, HASH, TRNG, PKA etc.) + * are at an offsets from the base (depending on SSS verion) + */ + unsigned int aes_offset;
This is a candidate to be stored inside a variant struct.
quoted hunk ↗ jump to hunk
}; static struct s5p_aes_dev *s5p_dev;@@ -358,14 +370,14 @@ static void s5p_set_aes(struct s5p_aes_dev *dev, { void __iomem *keystart; - memcpy(dev->ioaddr + SSS_REG_AES_IV_DATA(0), iv, 0x10); + memcpy(dev->ioaddr + SSS_REG_AES_IV_DATA(dev, 0), iv, 0x10); if (keylen == AES_KEYSIZE_256) - keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(0); + keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(dev, 0); else if (keylen == AES_KEYSIZE_192) - keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(2); + keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(dev, 2); else - keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(4); + keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(dev, 4); memcpy(keystart, key, keylen); }@@ -415,7 +427,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode) if (err) goto outdata_error; - SSS_WRITE(dev, AES_CONTROL, aes_control); + SSS_AES_WRITE(dev, AES_CONTROL, aes_control); s5p_set_aes(dev, dev->ctx->aes_key, req->info, dev->ctx->keylen); s5p_set_dma_indata(dev, req->src);@@ -618,6 +630,11 @@ static int s5p_aes_probe(struct platform_device *pdev) pdata->version = find_s5p_sss_version(pdev); + if (pdata->version == SSS_VER_5) + pdata->aes_offset = 0x200; + else + pdata->aes_offset = 0x4000; +
Using a variant struct if clauses like this would be eliminated.
quoted hunk ↗ jump to hunk
pdata->clk = devm_clk_get(dev, "secss"); if (IS_ERR(pdata->clk)) { dev_err(dev, "failed to find secss clock source\n");@@ -643,17 +660,23 @@ static int s5p_aes_probe(struct platform_device *pdev) goto err_irq; } + /* + * SSS module present in Exynos5 Series SoCs + * does not use hash interrupt + */ pdata->irq_hash = platform_get_irq(pdev, 1); - if (pdata->irq_hash < 0) { + if ((pdata->version == SSS_VER_4) && (pdata->irq_hash < 0)) {
A boolean field "has_hash_irq" in a variant struct could be used to tell whether this interrupt is needed instead of relying on version. Best regards, Tomasz