Re: [PATCH 3/8 v3] crypto:s5p-sss: Add support for SSS module on Exynos

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Naveen,

Please see my comments inline.

On 10.01.2014 12:42, Naveen Krishna Chatradhi wrote:
This patch adds new compatible and variant struct to support the SSS
module on Exynos4 (Exynos4210), Exynos5 (Exynos5420 and Exynos5250)
for which
1. AES register are at an offset of 0x200 and
2. hash interrupt is not available

Signed-off-by: Naveen Krishna Ch <ch.naveen@xxxxxxxxxxx>
CC: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
CC: David S. Miller <davem@xxxxxxxxxxxxx>
CC: Vladimir Zapolskiy <vzapolskiy@xxxxxxxxx>
TO: <linux-crypto@xxxxxxxxxxxxxxx>
CC: <linux-samsung-soc@xxxxxxxxxxxxxxx>
---
Changes since v2:
1. Added variant struct to handle the differences in SSS modules
2. Changed the compatible strings to exynos4210-secss
3. Other changes suggested by Tomasz

  .../devicetree/bindings/crypto/samsung-sss.txt     |   20 ++++
  drivers/crypto/s5p-sss.c                           |  110 +++++++++++++++-----
  2 files changed, 106 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/crypto/samsung-sss.txt b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
index 2f9d7e4..fdc7d8b 100644
--- a/Documentation/devicetree/bindings/crypto/samsung-sss.txt
+++ b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
@@ -8,13 +8,33 @@ The SSS module in S5PV210 SoC supports the following:
  -- SHA-1/SHA-256/MD5/HMAC (SHA-1/SHA-256/MD5)/PRNG
  -- PRNG: Pseudo Random Number Generator

+The SSS module in Exynos4 (Exynos4210) and
+Exynos5 (Exynos5420 and Exynos5250) SoCs
+supports the following also:
+-- ARCFOUR (ARC4)
+-- True Random Number Generator (TRNG)
+-- Secure Key Manager
+
  Required properties:

  - compatible : Should contain entries for this and backward compatible
    SSS versions:
    - "samsung,s5pv210-secss" for S5PV210 SoC.
+  - "samsung,exynos4210-secss" for Exynos4210, Exynos5250 and Exynos5420 SoCs.

You can also add Exynos4212/4412 to the list.

  - reg : Offset and length of the register set for the module
  - interrupts : the interrupt-specifier for the SSS module.
  		Two interrupts "feed control and hash" in case of S5PV210
+		One interrupts "feed control" in case of Exynos4210,
+			Exynos5250 and Exynos5420 SoCs.

You can refer to compatible string here instead of listing all the SoCs.

  - clocks : the required gating clock for the SSS module.
  - clock-names : the gating clock name to be requested in the SSS driver.

Again, please specify name of the clock in property description. The proper description for both clock properties should be:

- clock-names : list of device clock input names; should contain one entry - "secss". - clocks : list of clock phandle and specifier pairs for all clocks listed in clock-names property.

+
+Example:
+	/* SSS_VER_5 */
+	sss@10830000 {
+		compatible = "samsung,exynos4210-secss";
+		reg = <0x10830000 0x10000>;
+		interrupts = <0 112 0>;
+		clocks = <&clock 471>;
+		clock-names = "secss";
+	};
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 2da5617..f274f5f 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		0x00
  #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		0x04
  #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(off, s)	((off + 0x10) + (s << 2))
+#define SSS_REG_AES_OUT_DATA(off, s)	((off + 0x20) + (s << 2))
+#define SSS_REG_AES_IV_DATA(off, s)	((off + 0x30) + (s << 2))
+#define SSS_REG_AES_CNT_DATA(off, s)	((off + 0x40) + (s << 2))
+#define SSS_REG_AES_KEY_DATA(off, s)	((off + 0x80) + (s << 2))

I still somehow don't like this. Such macros are only hiding operations performed by the driver. See my comment below, in the code that references them, to see my proposal.


  #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->variant->aes_offset)
+#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)
@@ -146,6 +151,20 @@
  #define AES_KEY_LEN         16
  #define CRYPTO_QUEUE_LEN    1

+/**
+ * struct samsung_aes_variant - platform specific SSS driver data
+ * @has_hash_irq: true if SSS module uses hash interrupt, false otherwise
+ * @aes_offset: AES register offset from SSS module's base.
+ *
+ * Specifies platform specific configuration of SSS module.
+ * Note: A structure for driver specific platform data is used for future
+ * expansion of its usage.
+ */
+struct samsung_aes_variant {
+	bool			    has_hash_irq;
+	unsigned int		    aes_offset;
+};
+
  struct s5p_aes_reqctx {
  	unsigned long mode;
  };
@@ -174,16 +193,48 @@ struct s5p_aes_dev {
  	struct crypto_queue         queue;
  	bool                        busy;
  	spinlock_t                  lock;
+
+	struct samsung_aes_variant *variant;
  };

  static struct s5p_aes_dev *s5p_dev;

+static const struct samsung_aes_variant s5p_aes_data = {
+	.has_hash_irq	= true,
+	.aes_offset	= 0x4000,
+};
+
+static const struct samsung_aes_variant exynos_aes_data = {
+	.has_hash_irq	= false,
+	.aes_offset	= 0x200,
+};
+
  static const struct of_device_id s5p_sss_dt_match[] = {
-	{ .compatible = "samsung,s5pv210-secss", },
+	{
+		.compatible = "samsung,s5pv210-secss",
+		.data = &s5p_aes_data,
+	},
+	{
+		.compatible = "samsung,exynos4210-secss",
+		.data = &exynos_aes_data,
+	},
  	{ },
  };
  MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);

+static inline struct samsung_aes_variant *find_s5p_sss_version
+				   (struct platform_device *pdev)
+{
+	if (IS_ENABLED(CONFIG_OF) && (pdev->dev.of_node)) {
+		const struct of_device_id *match;
+		match = of_match_node(s5p_sss_dt_match,
+					pdev->dev.of_node);
+		return (struct samsung_aes_variant *)match->data;
+	}
+	return (struct samsung_aes_variant *)
+			platform_get_device_id(pdev)->driver_data;
+}
+
  static void s5p_set_dma_indata(struct s5p_aes_dev *dev, struct scatterlist *sg)
  {
  	SSS_WRITE(dev, FCBRDMAS, sg_dma_address(sg));
@@ -327,16 +378,21 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
  static void s5p_set_aes(struct s5p_aes_dev *dev,
  			uint8_t *key, uint8_t *iv, unsigned int keylen)
  {
+	struct samsung_aes_variant *var = dev->variant;
  	void __iomem *keystart;

-	memcpy(dev->ioaddr + SSS_REG_AES_IV_DATA(0), iv, 0x10);
+	memcpy(dev->ioaddr + SSS_REG_AES_IV_DATA
+				(var->aes_offset, 0), iv, 0x10);

What about adding aes_ioaddr to s5p_aes_dev struct? Then you could access the registers as follows:

	memcpy(dev->aes_ioaddr + SSS_REG_AES_IV_DATA(0), iv, 0x10);

The registers would be defined as offsets of AES base, e.g:

#define SSS_REG_AES_IV_DATA(s)		(0x10 + (s << 2))


  	if (keylen == AES_KEYSIZE_256)
-		keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(0);
+		keystart = dev->ioaddr +
+				SSS_REG_AES_KEY_DATA(var->aes_offset, 0);
  	else if (keylen == AES_KEYSIZE_192)
-		keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(2);
+		keystart = dev->ioaddr +
+				SSS_REG_AES_KEY_DATA(var->aes_offset, 2);
  	else
-		keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(4);
+		keystart = dev->ioaddr +
+				SSS_REG_AES_KEY_DATA(var->aes_offset, 4);

  	memcpy(keystart, key, keylen);
  }
@@ -386,7 +442,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);

SSS_AES_WRITE would be define using dev->aes_ioaddr instead of dev->ioaddr.

Otherwise the patch looks fine.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux