Date:	Mon, 13 Jan 2014 23:06:33 +0200
From:	Vladimir Zapolskiy <vz@...ia.com>
To:	Tomasz Figa <t.figa@...sung.com>
CC:	Naveen Krishna Chatradhi <ch.naveen@...sung.com>,
	linux-crypto@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
	linux-kernel@...r.kernel.org, herbert@...dor.apana.org.au,
	naveenkrishna.ch@...il.com, cpgs@...sung.com,
	tomasz.figa@...il.com, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 3/8 v3] crypto:s5p-sss: Add support for SSS module on
 Exynos

Hi Naveen and Tomasz,

On 01/10/14 17:44, Tomasz Figa wrote:
> 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@...sung.com>
>> CC: Herbert Xu <herbert@...dor.apana.org.au>
>> CC: David S. Miller <davem@...emloft.net>
>> CC: Vladimir Zapolskiy <vzapolskiy@...il.com>
>> TO: <linux-crypto@...r.kernel.org>
>> CC: <linux-samsung-soc@...r.kernel.org>
>> ---
>> 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@...30000 {
>> + 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))

I agree, this variant is more preferable.

>>
>> 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.
>

Same to me.

With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists