diff mbox

[2/6,v2] crypto:s5p-sss: Add device tree support

Message ID 1389243541-13122-1-git-send-email-ch.naveen@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Naveen Krishna Chatradhi Jan. 9, 2014, 4:59 a.m. UTC
This patch adds device tree support to the s5p-sss.c crypto driver.
Implements a varient struct to address the changes in SSS hardware
on various SoCs from Samsung.

Also, Documentation under devicetree/bindings added.

Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: David S. Miller <davem@davemloft.net>
CC: Vladimir Zapolskiy <vzapolskiy@gmail.com>
TO: <linux-crypto@vger.kernel.org>
CC: <linux-samsung-soc@vger.kernel.org>
---
Changes since v1:
1. Added description of the SSS module in Documentation
2. Corrected the interrupts description
3. Added varient struct instead of the version variable

 .../devicetree/bindings/crypto/samsung-sss.txt     |   20 +++++
 drivers/crypto/s5p-sss.c                           |   81 ++++++++++++++++++--
 2 files changed, 95 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/crypto/samsung-sss.txt

Comments

Tomasz Figa Jan. 9, 2014, 2:14 p.m. UTC | #1
Hi Naveen,

On 09.01.2014 05:59, Naveen Krishna Chatradhi wrote:
> This patch adds device tree support to the s5p-sss.c crypto driver.
> Implements a varient struct to address the changes in SSS hardware
> on various SoCs from Samsung.
>
> Also, Documentation under devicetree/bindings added.
>
> Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: David S. Miller <davem@davemloft.net>
> CC: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> TO: <linux-crypto@vger.kernel.org>
> CC: <linux-samsung-soc@vger.kernel.org>
> ---
> Changes since v1:
> 1. Added description of the SSS module in Documentation
> 2. Corrected the interrupts description
> 3. Added varient struct instead of the version variable
>
>   .../devicetree/bindings/crypto/samsung-sss.txt     |   20 +++++
>   drivers/crypto/s5p-sss.c                           |   81 ++++++++++++++++++--
>   2 files changed, 95 insertions(+), 6 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/crypto/samsung-sss.txt
>
> diff --git a/Documentation/devicetree/bindings/crypto/samsung-sss.txt b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
> new file mode 100644
> index 0000000..0e45b0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
> @@ -0,0 +1,20 @@
> +Samsung SoC SSS (Security SubSystem) module
> +
> +The SSS module in S5PV210 SoC supports the following:
> +-- Feeder (FeedCtrl)
> +-- Advanced Encryption Standard (AES)
> +-- Data Encryption Standard (DES)/3DES
> +-- Public Key Accelerator (PKA)
> +-- SHA-1/SHA-256/MD5/HMAC (SHA-1/SHA-256/MD5)/PRNG
> +-- PRNG: Pseudo Random Number Generator
> +
> +Required properties:
> +
> +- compatible : Should contain entries for this and backward compatible
> +  SSS versions:
> +  - "samsung,s5p-secss" for S5PV210 SoC.

As I wrote in my previous reply, please use specific compatible string 
containing name of SoC on which the block described by it appeared 
first. Let me say it again, for security block that showed up first on 
S5PV210 the string will be "samsung,s5pv210-secss".

> +- 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
> +- clocks : the required gating clock for the SSS module.
> +- clock-names : the gating clock name to be requested in the SSS driver.
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> index 93cddeb..78e0c37 100644
> --- a/drivers/crypto/s5p-sss.c
> +++ b/drivers/crypto/s5p-sss.c
> @@ -22,6 +22,7 @@
>   #include <linux/scatterlist.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/io.h>
> +#include <linux/of.h>
>   #include <linux/crypto.h>
>   #include <linux/interrupt.h>
>
> @@ -145,6 +146,20 @@
>   #define AES_KEY_LEN         16
>   #define CRYPTO_QUEUE_LEN    1
>
> +/**
> + * struct samsung_aes_varient - platform specific SSS driver data

typo: s/varient/variant/g

Anyway, I think this should be moved to the patch adding support for 
Exynos5, as before it there is no use for this struct and it's not 
directly related to DT support.

> + * @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_varient {
> +	bool			    has_hash_irq;
> +	unsigned int		    aes_offset;
> +};
> +
>   struct s5p_aes_reqctx {
>   	unsigned long mode;
>   };
> @@ -173,10 +188,56 @@ struct s5p_aes_dev {
>   	struct crypto_queue         queue;
>   	bool                        busy;
>   	spinlock_t                  lock;
> +
> +	struct samsung_aes_varient *varient;
>   };
>
>   static struct s5p_aes_dev *s5p_dev;
>
> +static struct samsung_aes_varient s5p_aes_data = {

Remember to mark constant data as const.

> +	.has_hash_irq	= true,
> +	.aes_offset	= 0x4000,
> +};
> +
> +static const struct platform_device_id s5p_sss_ids[] = {
> +	{
> +		.name		= "s5p-secss",
> +		.driver_data	= (kernel_ulong_t)&s5p_aes_data,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(platform, s5p_sss_ids);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id s5p_sss_dt_match[] = {
> +	{
> +		.compatible = "samsung,s5p-secss",
> +		.data = (void *)&s5p_aes_data,

No need to cast pointers to (void *) explicitly.

> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
> +#else
> +static struct of_device_id s5p_sss_dt_match[] =  {
> +	{ },
> +};

Hmm, I don't think there is any need for this.

> +#endif
> +
> +static inline struct samsung_aes_varient *find_s5p_sss_version
> +				   (struct platform_device *pdev)
> +{
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		if (pdev->dev.of_node) {

What about using a single if, as follows:

if (IS_ENABLED(CONFIG_IF) && pdev->dev.of_node)

> +			const struct of_device_id *match;
> +			match = of_match_node(s5p_sss_dt_match,

You can use of_match_ptr(s5p_sss_dt_match) instead of referring to the 
table directly to eliminate the need for empty table.

> +						pdev->dev.of_node);
> +		return (struct samsung_aes_varient *)match->data;
> +		}
> +	}
> +	return (struct samsung_aes_varient *)
> +			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));
> @@ -564,6 +625,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
>   	struct s5p_aes_dev *pdata;
>   	struct device      *dev = &pdev->dev;
>   	struct resource    *res;
> +	struct samsung_aes_varient *varient;
>
>   	if (s5p_dev)
>   		return -EEXIST;
> @@ -580,6 +642,8 @@ static int s5p_aes_probe(struct platform_device *pdev)
>   				     resource_size(res), pdev->name))
>   		return -EBUSY;
>
> +	varient = find_s5p_sss_version(pdev);
> +
>   	pdata->clk = devm_clk_get(dev, "secss");
>   	if (IS_ERR(pdata->clk)) {
>   		dev_err(dev, "failed to find secss clock source\n");
> @@ -606,18 +670,21 @@ static int s5p_aes_probe(struct platform_device *pdev)
>   	}
>

The if (variant->has_hash_irq) should start here and cover the whole 
block handling second interrupt. This should be more readable.

>   	pdata->irq_hash = platform_get_irq(pdev, 1);
> -	if (pdata->irq_hash < 0) {
> +	if (varient->has_hash_irq && pdata->irq_hash < 0) {
>   		err = pdata->irq_hash;
>   		dev_warn(dev, "hash interrupt is not available.\n");
>   		goto err_irq;
>   	}
> -	err = devm_request_irq(dev, pdata->irq_hash, s5p_aes_interrupt,
> -			       IRQF_SHARED, pdev->name, pdev);
> -	if (err < 0) {
> -		dev_warn(dev, "hash interrupt is not available.\n");
> -		goto err_irq;
> +	if (varient->has_hash_irq) {
> +		err = devm_request_irq(dev, pdata->irq_hash, s5p_aes_interrupt,
> +				       IRQF_SHARED, pdev->name, pdev);
> +		if (err < 0) {
> +			dev_warn(dev, "hash interrupt is not available.\n");
> +			goto err_irq;
> +		}
>   	}
>
> +	pdata->varient = varient;
>   	pdata->dev = dev;
>   	platform_set_drvdata(pdev, pdata);
>   	s5p_dev = pdata;
> @@ -674,9 +741,11 @@ static int s5p_aes_remove(struct platform_device *pdev)
>   static struct platform_driver s5p_aes_crypto = {
>   	.probe	= s5p_aes_probe,
>   	.remove	= s5p_aes_remove,
> +	.id_table	= s5p_sss_ids,
>   	.driver	= {
>   		.owner	= THIS_MODULE,
>   		.name	= "s5p-secss",
> +		.of_match_table = s5p_sss_dt_match,

.of_match_table = of_match_ptr(s5p_sss_dt_match),

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Naveen Krishna Ch Jan. 10, 2014, 6:07 a.m. UTC | #2
Hello Tomasz,

Thanks for time and review comments they are really helping me a lot
in getting the patches merged.

Secondly, accept my apologies for not giving proper writeup of why i
din't address
few of your comments on v1 of this patchset.

On 9 January 2014 19:44, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Naveen,
>
>
> On 09.01.2014 05:59, Naveen Krishna Chatradhi wrote:
>>
>> This patch adds device tree support to the s5p-sss.c crypto driver.
>> Implements a varient struct to address the changes in SSS hardware
>> on various SoCs from Samsung.
>>
>> Also, Documentation under devicetree/bindings added.
>>
>> Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
>> CC: Herbert Xu <herbert@gondor.apana.org.au>
>> CC: David S. Miller <davem@davemloft.net>
>> CC: Vladimir Zapolskiy <vzapolskiy@gmail.com>
>> TO: <linux-crypto@vger.kernel.org>
>> CC: <linux-samsung-soc@vger.kernel.org>
>> ---
>> Changes since v1:
>> 1. Added description of the SSS module in Documentation
>> 2. Corrected the interrupts description
>> 3. Added varient struct instead of the version variable
>>
>>   .../devicetree/bindings/crypto/samsung-sss.txt     |   20 +++++
>>   drivers/crypto/s5p-sss.c                           |   81
>> ++++++++++++++++++--
>>   2 files changed, 95 insertions(+), 6 deletions(-)
>>   create mode 100644
>> Documentation/devicetree/bindings/crypto/samsung-sss.txt
>>
>> diff --git a/Documentation/devicetree/bindings/crypto/samsung-sss.txt
>> b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
>> new file mode 100644
>> index 0000000..0e45b0d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
>> @@ -0,0 +1,20 @@
>> +Samsung SoC SSS (Security SubSystem) module
>> +
>> +The SSS module in S5PV210 SoC supports the following:
>> +-- Feeder (FeedCtrl)
>> +-- Advanced Encryption Standard (AES)
>> +-- Data Encryption Standard (DES)/3DES
>> +-- Public Key Accelerator (PKA)
>> +-- SHA-1/SHA-256/MD5/HMAC (SHA-1/SHA-256/MD5)/PRNG
>> +-- PRNG: Pseudo Random Number Generator
>> +
>> +Required properties:
>> +
>> +- compatible : Should contain entries for this and backward compatible
>> +  SSS versions:
>> +  - "samsung,s5p-secss" for S5PV210 SoC.
>
>
> As I wrote in my previous reply, please use specific compatible string
> containing name of SoC on which the block described by it appeared first.
> Let me say it again, for security block that showed up first on S5PV210 the
> string will be "samsung,s5pv210-secss".
1. .name           = "s5p-secss", is the name used in the present driver.
    So, i dint modify that.
2. I'm not sure which one is the first soc to have the new varient.
    May be Exynos4210. Will modify in the next version.

>
>
>> +- 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
>> +- clocks : the required gating clock for the SSS module.
>> +- clock-names : the gating clock name to be requested in the SSS driver.
>> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
>> index 93cddeb..78e0c37 100644
>> --- a/drivers/crypto/s5p-sss.c
>> +++ b/drivers/crypto/s5p-sss.c
>> @@ -22,6 +22,7 @@
>>   #include <linux/scatterlist.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/io.h>
>> +#include <linux/of.h>
>>   #include <linux/crypto.h>
>>   #include <linux/interrupt.h>
>>
>> @@ -145,6 +146,20 @@
>>   #define AES_KEY_LEN         16
>>   #define CRYPTO_QUEUE_LEN    1
>>
>> +/**
>> + * struct samsung_aes_varient - platform specific SSS driver data
>
>
> typo: s/varient/variant/g
>
> Anyway, I think this should be moved to the patch adding support for
> Exynos5, as before it there is no use for this struct and it's not directly
> related to DT support.
>
>
>> + * @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_varient {
>> +       bool                        has_hash_irq;
>> +       unsigned int                aes_offset;
>> +};
>> +
>>   struct s5p_aes_reqctx {
>>         unsigned long mode;
>>   };
>> @@ -173,10 +188,56 @@ struct s5p_aes_dev {
>>         struct crypto_queue         queue;
>>         bool                        busy;
>>         spinlock_t                  lock;
>> +
>> +       struct samsung_aes_varient *varient;
>>   };
>>
>>   static struct s5p_aes_dev *s5p_dev;
>>
>> +static struct samsung_aes_varient s5p_aes_data = {
>
>
> Remember to mark constant data as const.
>
>
>> +       .has_hash_irq   = true,
>> +       .aes_offset     = 0x4000,
>> +};
>> +
>> +static const struct platform_device_id s5p_sss_ids[] = {
>> +       {
>> +               .name           = "s5p-secss",
>> +               .driver_data    = (kernel_ulong_t)&s5p_aes_data,
>> +       },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(platform, s5p_sss_ids);
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id s5p_sss_dt_match[] = {
>> +       {
>> +               .compatible = "samsung,s5p-secss",
>> +               .data = (void *)&s5p_aes_data,
>
>
> No need to cast pointers to (void *) explicitly.
>
>
>> +       },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
>> +#else
>> +static struct of_device_id s5p_sss_dt_match[] =  {
>> +       { },
>> +};
>
>
> Hmm, I don't think there is any need for this.
I think all Exynos4 and Exynos5 now supports DT
But, i'm not sure of the S5PV210 series.
>
>
>> +#endif
>> +
>> +static inline struct samsung_aes_varient *find_s5p_sss_version
>> +                                  (struct platform_device *pdev)
>> +{
>> +       if (IS_ENABLED(CONFIG_OF)) {
>> +               if (pdev->dev.of_node) {
>
>
> What about using a single if, as follows:
>
> if (IS_ENABLED(CONFIG_IF) && pdev->dev.of_node)
>
>
>> +                       const struct of_device_id *match;
>> +                       match = of_match_node(s5p_sss_dt_match,
>
>
> You can use of_match_ptr(s5p_sss_dt_match) instead of referring to the table
> directly to eliminate the need for empty table.
>
>
>> +                                               pdev->dev.of_node);
>> +               return (struct samsung_aes_varient *)match->data;
>> +               }
>> +       }
>> +       return (struct samsung_aes_varient *)
>> +                       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));
>> @@ -564,6 +625,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
>>         struct s5p_aes_dev *pdata;
>>         struct device      *dev = &pdev->dev;
>>         struct resource    *res;
>> +       struct samsung_aes_varient *varient;
>>
>>         if (s5p_dev)
>>                 return -EEXIST;
>> @@ -580,6 +642,8 @@ static int s5p_aes_probe(struct platform_device *pdev)
>>                                      resource_size(res), pdev->name))
>>                 return -EBUSY;
>>
>> +       varient = find_s5p_sss_version(pdev);
>> +
>>         pdata->clk = devm_clk_get(dev, "secss");
>>         if (IS_ERR(pdata->clk)) {
>>                 dev_err(dev, "failed to find secss clock source\n");
>> @@ -606,18 +670,21 @@ static int s5p_aes_probe(struct platform_device
>> *pdev)
>>         }
>>
>
> The if (variant->has_hash_irq) should start here and cover the whole block
> handling second interrupt. This should be more readable.
Indeed it is, But, adding extra tab was causing more lines to cross 80char limit
so, I used it this way.. Will modify in the next version.
>
>
>>         pdata->irq_hash = platform_get_irq(pdev, 1);
>> -       if (pdata->irq_hash < 0) {
>> +       if (varient->has_hash_irq && pdata->irq_hash < 0) {
>>                 err = pdata->irq_hash;
>>                 dev_warn(dev, "hash interrupt is not available.\n");
>>                 goto err_irq;
>>         }
>> -       err = devm_request_irq(dev, pdata->irq_hash, s5p_aes_interrupt,
>> -                              IRQF_SHARED, pdev->name, pdev);
>> -       if (err < 0) {
>> -               dev_warn(dev, "hash interrupt is not available.\n");
>> -               goto err_irq;
>> +       if (varient->has_hash_irq) {
>> +               err = devm_request_irq(dev, pdata->irq_hash,
>> s5p_aes_interrupt,
>> +                                      IRQF_SHARED, pdev->name, pdev);
>> +               if (err < 0) {
>> +                       dev_warn(dev, "hash interrupt is not
>> available.\n");
>> +                       goto err_irq;
>> +               }
>>         }
>>
>> +       pdata->varient = varient;
>>         pdata->dev = dev;
>>         platform_set_drvdata(pdev, pdata);
>>         s5p_dev = pdata;
>> @@ -674,9 +741,11 @@ static int s5p_aes_remove(struct platform_device
>> *pdev)
>>   static struct platform_driver s5p_aes_crypto = {
>>         .probe  = s5p_aes_probe,
>>         .remove = s5p_aes_remove,
>> +       .id_table       = s5p_sss_ids,
>>         .driver = {
>>                 .owner  = THIS_MODULE,
>>                 .name   = "s5p-secss",
>> +               .of_match_table = s5p_sss_dt_match,
>
>
> .of_match_table = of_match_ptr(s5p_sss_dt_match),
I dint use it, Some time back there was a patchset from Sachin
https://lkml.org/lkml/2013/9/28/61
Please suggest me on this.
>
> Best regards,
> Tomasz
Sachin Kamat Jan. 10, 2014, 6:20 a.m. UTC | #3
Hi Naveen,

On 10 January 2014 11:37, Naveen Krishna Ch <naveenkrishna.ch@gmail.com> wrote:
> Hello Tomasz,
>

[snip]
>>> *pdev)
>>>   static struct platform_driver s5p_aes_crypto = {
>>>         .probe  = s5p_aes_probe,
>>>         .remove = s5p_aes_remove,
>>> +       .id_table       = s5p_sss_ids,
>>>         .driver = {
>>>                 .owner  = THIS_MODULE,
>>>                 .name   = "s5p-secss",
>>> +               .of_match_table = s5p_sss_dt_match,
>>
>>
>> .of_match_table = of_match_ptr(s5p_sss_dt_match),
> I dint use it, Some time back there was a patchset from Sachin
> https://lkml.org/lkml/2013/9/28/61
> Please suggest me on this.

In those cases the structure was always compiled in. i.e., it was not protected
by #ifdef CONFIG_OF. Hence use of of_match_ptr was not required. of_match_ptr
abstracts this check depending on OF is enabled or not. In the case of
this (sss) driver,
since you are using CONFIG_OF to selectively compile out the code (and esp.
s5p_sss_dt_match structure), use of of_match_ptr will make the code
simpler as it
avoids the use of a dummy structure definition (the #else part of the
struct definition) when OF is
disabled. Hope this clarifies.
Tomasz Figa Jan. 10, 2014, 1:44 p.m. UTC | #4
Hi Naveen,

On 10.01.2014 07:07, Naveen Krishna Ch wrote:
> Hello Tomasz,
>
> Thanks for time and review comments they are really helping me a lot
> in getting the patches merged.
>
> Secondly, accept my apologies for not giving proper writeup of why i
> din't address
> few of your comments on v1 of this patchset.

It's okay.

>
> On 9 January 2014 19:44, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Naveen,
>>
>>
>> On 09.01.2014 05:59, Naveen Krishna Chatradhi wrote:
>>>
>>> This patch adds device tree support to the s5p-sss.c crypto driver.
>>> Implements a varient struct to address the changes in SSS hardware
>>> on various SoCs from Samsung.
>>>
>>> Also, Documentation under devicetree/bindings added.
>>>
>>> Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
>>> CC: Herbert Xu <herbert@gondor.apana.org.au>
>>> CC: David S. Miller <davem@davemloft.net>
>>> CC: Vladimir Zapolskiy <vzapolskiy@gmail.com>
>>> TO: <linux-crypto@vger.kernel.org>
>>> CC: <linux-samsung-soc@vger.kernel.org>
>>> ---
>>> Changes since v1:
>>> 1. Added description of the SSS module in Documentation
>>> 2. Corrected the interrupts description
>>> 3. Added varient struct instead of the version variable
>>>
>>>    .../devicetree/bindings/crypto/samsung-sss.txt     |   20 +++++
>>>    drivers/crypto/s5p-sss.c                           |   81
>>> ++++++++++++++++++--
>>>    2 files changed, 95 insertions(+), 6 deletions(-)
>>>    create mode 100644
>>> Documentation/devicetree/bindings/crypto/samsung-sss.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/crypto/samsung-sss.txt
>>> b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
>>> new file mode 100644
>>> index 0000000..0e45b0d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
>>> @@ -0,0 +1,20 @@
>>> +Samsung SoC SSS (Security SubSystem) module
>>> +
>>> +The SSS module in S5PV210 SoC supports the following:
>>> +-- Feeder (FeedCtrl)
>>> +-- Advanced Encryption Standard (AES)
>>> +-- Data Encryption Standard (DES)/3DES
>>> +-- Public Key Accelerator (PKA)
>>> +-- SHA-1/SHA-256/MD5/HMAC (SHA-1/SHA-256/MD5)/PRNG
>>> +-- PRNG: Pseudo Random Number Generator
>>> +
>>> +Required properties:
>>> +
>>> +- compatible : Should contain entries for this and backward compatible
>>> +  SSS versions:
>>> +  - "samsung,s5p-secss" for S5PV210 SoC.
>>
>>
>> As I wrote in my previous reply, please use specific compatible string
>> containing name of SoC on which the block described by it appeared first.
>> Let me say it again, for security block that showed up first on S5PV210 the
>> string will be "samsung,s5pv210-secss".
> 1. .name           = "s5p-secss", is the name used in the present driver.
>      So, i dint modify that.

Please note that compatible strings and platform driver names are 
completely different things. There is no relation between them. 
Moreover, there are different rules (or recommendation) involved for 
creating both, so it's completely fine to add a compatible string of 
"samsung,s5pv210-secss", while keeping platform driver named "s5p-secss".

> 2. I'm not sure which one is the first soc to have the new varient.
>      May be Exynos4210. Will modify in the next version.

Let's see.

 From what I can find in various user's manuals, S5PV210 is the first to 
have the first variant, while Exynos4210 already has the new one.

>>
>>> +       },
>>> +       { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
>>> +#else
>>> +static struct of_device_id s5p_sss_dt_match[] =  {
>>> +       { },
>>> +};
>>
>>
>> Hmm, I don't think there is any need for this.
> I think all Exynos4 and Exynos5 now supports DT
> But, i'm not sure of the S5PV210 series.

S5PV210 does not support DT yet. Work is already in progress, but board 
file support will have to be retained for some time, so this driver 
should support non-DT probing too.

It doesn't affect my comment, though, as you can use of_match_ptr() macro.

>>> @@ -674,9 +741,11 @@ static int s5p_aes_remove(struct platform_device
>>> *pdev)
>>>    static struct platform_driver s5p_aes_crypto = {
>>>          .probe  = s5p_aes_probe,
>>>          .remove = s5p_aes_remove,
>>> +       .id_table       = s5p_sss_ids,
>>>          .driver = {
>>>                  .owner  = THIS_MODULE,
>>>                  .name   = "s5p-secss",
>>> +               .of_match_table = s5p_sss_dt_match,
>>
>>
>> .of_match_table = of_match_ptr(s5p_sss_dt_match),
> I dint use it, Some time back there was a patchset from Sachin
> https://lkml.org/lkml/2013/9/28/61
> Please suggest me on this.

I believe Sachin already explained this in his reply to your patch. 
Generally the driver from your link is supposed to always support DT, 
while this one can be built without DT support.

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

Patch

diff --git a/Documentation/devicetree/bindings/crypto/samsung-sss.txt b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
new file mode 100644
index 0000000..0e45b0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
@@ -0,0 +1,20 @@ 
+Samsung SoC SSS (Security SubSystem) module
+
+The SSS module in S5PV210 SoC supports the following:
+-- Feeder (FeedCtrl)
+-- Advanced Encryption Standard (AES)
+-- Data Encryption Standard (DES)/3DES
+-- Public Key Accelerator (PKA)
+-- SHA-1/SHA-256/MD5/HMAC (SHA-1/SHA-256/MD5)/PRNG
+-- PRNG: Pseudo Random Number Generator
+
+Required properties:
+
+- compatible : Should contain entries for this and backward compatible
+  SSS versions:
+  - "samsung,s5p-secss" for S5PV210 SoC.
+- 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
+- clocks : the required gating clock for the SSS module.
+- clock-names : the gating clock name to be requested in the SSS driver.
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 93cddeb..78e0c37 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -22,6 +22,7 @@ 
 #include <linux/scatterlist.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
+#include <linux/of.h>
 #include <linux/crypto.h>
 #include <linux/interrupt.h>
 
@@ -145,6 +146,20 @@ 
 #define AES_KEY_LEN         16
 #define CRYPTO_QUEUE_LEN    1
 
+/**
+ * struct samsung_aes_varient - 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_varient {
+	bool			    has_hash_irq;
+	unsigned int		    aes_offset;
+};
+
 struct s5p_aes_reqctx {
 	unsigned long mode;
 };
@@ -173,10 +188,56 @@  struct s5p_aes_dev {
 	struct crypto_queue         queue;
 	bool                        busy;
 	spinlock_t                  lock;
+
+	struct samsung_aes_varient *varient;
 };
 
 static struct s5p_aes_dev *s5p_dev;
 
+static struct samsung_aes_varient s5p_aes_data = {
+	.has_hash_irq	= true,
+	.aes_offset	= 0x4000,
+};
+
+static const struct platform_device_id s5p_sss_ids[] = {
+	{
+		.name		= "s5p-secss",
+		.driver_data	= (kernel_ulong_t)&s5p_aes_data,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, s5p_sss_ids);
+
+#ifdef CONFIG_OF
+static const struct of_device_id s5p_sss_dt_match[] = {
+	{
+		.compatible = "samsung,s5p-secss",
+		.data = (void *)&s5p_aes_data,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
+#else
+static struct of_device_id s5p_sss_dt_match[] =  {
+	{ },
+};
+#endif
+
+static inline struct samsung_aes_varient *find_s5p_sss_version
+				   (struct platform_device *pdev)
+{
+	if (IS_ENABLED(CONFIG_OF)) {
+		if (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_varient *)match->data;
+		}
+	}
+	return (struct samsung_aes_varient *)
+			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));
@@ -564,6 +625,7 @@  static int s5p_aes_probe(struct platform_device *pdev)
 	struct s5p_aes_dev *pdata;
 	struct device      *dev = &pdev->dev;
 	struct resource    *res;
+	struct samsung_aes_varient *varient;
 
 	if (s5p_dev)
 		return -EEXIST;
@@ -580,6 +642,8 @@  static int s5p_aes_probe(struct platform_device *pdev)
 				     resource_size(res), pdev->name))
 		return -EBUSY;
 
+	varient = find_s5p_sss_version(pdev);
+
 	pdata->clk = devm_clk_get(dev, "secss");
 	if (IS_ERR(pdata->clk)) {
 		dev_err(dev, "failed to find secss clock source\n");
@@ -606,18 +670,21 @@  static int s5p_aes_probe(struct platform_device *pdev)
 	}
 
 	pdata->irq_hash = platform_get_irq(pdev, 1);
-	if (pdata->irq_hash < 0) {
+	if (varient->has_hash_irq && pdata->irq_hash < 0) {
 		err = pdata->irq_hash;
 		dev_warn(dev, "hash interrupt is not available.\n");
 		goto err_irq;
 	}
-	err = devm_request_irq(dev, pdata->irq_hash, s5p_aes_interrupt,
-			       IRQF_SHARED, pdev->name, pdev);
-	if (err < 0) {
-		dev_warn(dev, "hash interrupt is not available.\n");
-		goto err_irq;
+	if (varient->has_hash_irq) {
+		err = devm_request_irq(dev, pdata->irq_hash, s5p_aes_interrupt,
+				       IRQF_SHARED, pdev->name, pdev);
+		if (err < 0) {
+			dev_warn(dev, "hash interrupt is not available.\n");
+			goto err_irq;
+		}
 	}
 
+	pdata->varient = varient;
 	pdata->dev = dev;
 	platform_set_drvdata(pdev, pdata);
 	s5p_dev = pdata;
@@ -674,9 +741,11 @@  static int s5p_aes_remove(struct platform_device *pdev)
 static struct platform_driver s5p_aes_crypto = {
 	.probe	= s5p_aes_probe,
 	.remove	= s5p_aes_remove,
+	.id_table	= s5p_sss_ids,
 	.driver	= {
 		.owner	= THIS_MODULE,
 		.name	= "s5p-secss",
+		.of_match_table = s5p_sss_dt_match,
 	},
 };