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

[Naveen Krishna Ch]

Hell Vladimir, Tomasz,

On 14 January 2014 02:36, Vladimir Zapolskiy <vz@xxxxxxxxx> wrote:
> 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@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))
>
>
> I agree, this variant is more preferable.
Sure will implement it.
>
>
>>>
>>> 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.
Thanks for the review, Will implement these changes tomorrow.
>
> With best wishes,
> Vladimir



-- 
Shine bright,
(: Nav :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/