Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752827AbaAJPpG (ORCPT ); Fri, 10 Jan 2014 10:45:06 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:50405 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750944AbaAJPpB (ORCPT ); Fri, 10 Jan 2014 10:45:01 -0500 X-AuditID: cbfec7f5-b7fc96d000004885-3d-52d01578db56 Message-id: <52D01572.5010509@samsung.com> Date: Fri, 10 Jan 2014 16:44:50 +0100 From: Tomasz Figa User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.1 MIME-version: 1.0 To: Naveen Krishna Chatradhi , linux-crypto@vger.kernel.org, linux-samsung-soc@vger.kernel.org Cc: linux-kernel@vger.kernel.org, vzapolskiy@gmail.com, herbert@gondor.apana.org.au, naveenkrishna.ch@gmail.com, cpgs@samsung.com, tomasz.figa@gmail.com, "David S. Miller" Subject: Re: [PATCH 3/8 v3] crypto:s5p-sss: Add support for SSS module on Exynos References: <1389095509-14357-4-git-send-email-ch.naveen@samsung.com> <1389354171-31816-1-git-send-email-ch.naveen@samsung.com> In-reply-to: <1389354171-31816-1-git-send-email-ch.naveen@samsung.com> Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrFLMWRmVeSWpSXmKPExsVy+t/xK7qVoheCDE6nWtx9fpjR4uUhTYs5 51tYLLpfyVjcv/eTyeLyrjlsFjPO72OyWLTtP7PFql1/GC3OzjnE5MDlsWXlTSaPnbPusnts O6Dq0bdlFaPH501yAaxRXDYpqTmZZalF+nYJXBlXH31jLTjjUXFl+3n2BsalFl2MnBwSAiYS k1ZPYoawxSQu3FvP1sXIxSEksJRR4kXrM0YI5zOjxN01H8GqeAW0JK6cOM4IYrMIqEq8uP6H CcRmE1CT+NzwiA3EFhWIkPg7bz0jRL2gxI/J91hAbBGBcol/N9aAbWAWOMoo8XnORbBmYYEA ie6u+WC2kEAjo8TUVi8Qm1PATeLe83tgi5kFrCVWTtrGCGHLS2xe85Z5AqPALCQ7ZiEpm4Wk bAEj8ypG0dTS5ILipPRcI73ixNzi0rx0veT83E2MkPD/uoNx6TGrQ4wCHIxKPLw/BC8ECbEm lhVX5h5ilOBgVhLhvQYS4k1JrKxKLcqPLyrNSS0+xMjEwSnVwLiqLHCx53+PpCSHcPOZpU7e +zymbpqquGKe77n+Sl6Brce6+M1Ol0+0b5C9HM4a8u6uu8D6rUGfnfjvPI8677G5covjuoDw 7ntq6X0lnxZfyORK65St71+/j0XBcfLe9V2bdgQmxFurnYtazpTC+fnh/qzTN+a8zysPv/C3 5UWMjpnj22WBPkosxRmJhlrMRcWJAP5OXT1dAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > CC: Herbert Xu > CC: David S. Miller > CC: Vladimir Zapolskiy > TO: > CC: > --- > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/